Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt aggregated apiserver #1048

Merged
merged 15 commits into from
Oct 30, 2019
Merged

Adopt aggregated apiserver #1048

merged 15 commits into from
Oct 30, 2019

Conversation

aylei
Copy link
Contributor

@aylei aylei commented Oct 24, 2019

Signed-off-by: Aylei rayingecho@gmail.com

What problem does this PR solve?

#1002

What is changed and how does it work?

Add aggregated apiserver skeleton and toolchain.

More todos are listed in the original issue #1002

Check List

Tests

Manual test

  • Create
  • Update
  • List
  • Watch

Code changes

  • Has Go code change

Does this PR introduce a user-facing change?:

New component TiDB aggregated apiserver.

Here's an example about adding new resources in AA: aylei#1

To make review easier, I split this PR to multiple commits:

  • Pin the versions of dependencies for Aggregated Apiserver, which actually can be ignored: 9103ca8
  • Add dataresource CRD, which introduce a new CRD called DataResource as the storage of our AA: 9103ca8
  • Generate code for dataresouce CRD, which can be ignored: 92fad18
  • Important: Implement a kube-apiserver based storage.Interface: 209a6e1
  • Add aggregated apiserver skeleton and toolchain: 0fa2977

PTAL @weekface @cofyc @tennix @onlymellb @Yisaer @DanielZhangQD

@@ -0,0 +1,90 @@
# Adding New Resources in TiDB Aggregated Apiserver
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A brief introduction of the usage

}

// StartApiServer starts an apiserver.
func StartApiServer(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AA entrypoint


// Config create an apiserver config
func (o ServerOptions) Config() (*apiserver.Config, error) {
if err := o.RecommendedOptions.SecureServing.MaybeDefaultWithSelfSignedCerts(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CA issue is left for later resolution

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self-sign certs and skip tls verify should be enough for internal use

)

// store implements a ConfigMap backed storage.Interface
type store struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation of a kube-apiserver based storage.Interface, however, the implementation of watch is naive and will be improved in an separated PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add some unit tests to cover basic use cases?

@aylei
Copy link
Contributor Author

aylei commented Oct 24, 2019

/run-e2e-in-kind

@aylei aylei mentioned this pull request Oct 24, 2019
10 tasks
@aylei
Copy link
Contributor Author

aylei commented Oct 24, 2019

e2e is not working for this issue now, because the apiserver lib introduce a dependency bitbucket.org/ww/goautoneg that have to be downloaded thru hg, which is not installed in the CI env.

$ go mod why bitbucket.org/ww/goautoneg
# bitbucket.org/ww/goautoneg
github.com/pingcap/tidb-operator/pkg/apiserver/cmd
k8s.io/apiserver/pkg/endpoints/filters
k8s.io/apiserver/pkg/endpoints/handlers/responsewriters
k8s.io/apiserver/pkg/endpoints/handlers/negotiation
bitbucket.org/ww/goautoneg

I will fix it soon, it does not block reviewing, though.

@cofyc
Copy link
Contributor

cofyc commented Oct 25, 2019

/run-e2e-in-kind

1 similar comment
@cofyc
Copy link
Contributor

cofyc commented Oct 25, 2019

/run-e2e-in-kind

@cofyc
Copy link
Contributor

cofyc commented Oct 25, 2019

e2e is not working for this issue now, because the apiserver lib introduce a dependency bitbucket.org/ww/goautoneg that have to be downloaded thru hg, which is not installed in the CI env.

I've added a command to install hg in CI script.

@aylei
Copy link
Contributor Author

aylei commented Oct 25, 2019

@cofyc Thanks!

@@ -32,4 +32,5 @@ import (
_ "k8s.io/code-generator/cmd/openapi-gen"
_ "k8s.io/code-generator/cmd/register-gen"
_ "k8s.io/code-generator/cmd/set-gen"
_ "github.com/kubernetes-incubator/apiserver-builder-alpha/cmd/apiregister-gen"
Copy link
Contributor

@cofyc cofyc Oct 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use sigs.k8s.io/apiserver-builder-alpha/cmd/apiregister-gen?

this project is moved to https://github.com/kubernetes-sigs/apiserver-builder-alpha/ now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pinned version v1.12 was still using the kubernetes-incubator import path, we can use sigs.k8s.io.... after we bump the kubernetes dependencies to v1.16

@cofyc
Copy link
Contributor

cofyc commented Oct 28, 2019

because I renamed pkg/apis/pingcap.com to pkg/apis/pingcap, some conflicts must be resolved. regenerating code and updating imports should be enough

@aylei
Copy link
Contributor Author

aylei commented Oct 28, 2019

@cofyc Thanks for noting!

@aylei aylei requested review from DanielZhangQD and Yisaer October 28, 2019 07:32
aylei added 6 commits October 28, 2019 17:09
Signed-off-by: Aylei <rayingecho@gmail.com>
Signed-off-by: Aylei <rayingecho@gmail.com>
Signed-off-by: Aylei <rayingecho@gmail.com>
Signed-off-by: Aylei <rayingecho@gmail.com>
Signed-off-by: Aylei <rayingecho@gmail.com>
Signed-off-by: Aylei <rayingecho@gmail.com>
tennix
tennix previously approved these changes Oct 29, 2019
Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Aylei <rayingecho@gmail.com>
@aylei
Copy link
Contributor Author

aylei commented Oct 29, 2019

/run-e2e-tests

@aylei
Copy link
Contributor Author

aylei commented Oct 29, 2019

@cofyc @tennix @onlymellb @DanielZhangQD I've addressed the review comments and add the basic unit tests for store.go(626f3f0). Please take a look again and approve if there's no problem, thanks!

/run-e2e-in-kind

@aylei aylei mentioned this pull request Oct 29, 2019
Signed-off-by: Aylei <rayingecho@gmail.com>
@aylei
Copy link
Contributor Author

aylei commented Oct 29, 2019

/run-e2e-in-kind

Signed-off-by: Aylei <rayingecho@gmail.com>
@aylei
Copy link
Contributor Author

aylei commented Oct 29, 2019

/run-e2e-in-kind

3 similar comments
@aylei
Copy link
Contributor Author

aylei commented Oct 30, 2019

/run-e2e-in-kind

@cofyc
Copy link
Contributor

cofyc commented Oct 30, 2019

/run-e2e-in-kind

@cofyc
Copy link
Contributor

cofyc commented Oct 30, 2019

/run-e2e-in-kind

@aylei
Copy link
Contributor Author

aylei commented Oct 30, 2019

/run-e2e-in-kind

Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@DanielZhangQD DanielZhangQD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@cofyc cofyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aylei aylei merged commit fa36736 into pingcap:master Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants