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

Extend apiserver testserver #55548

Merged

Conversation

dhilipkumars
Copy link

@dhilipkumars dhilipkumars commented Nov 12, 2017

What this PR does / why we need it:
Extend the test pkg of api-server a little bit so that it can be used by others
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
We wanted to use this test pkg while creating a new integration test fixture, the idea of starting the api-server without actually starting a proper etcd (integration etcd ) is very useful, if would be great to extend this for other usecases such as insecure mode too.

It will also be useful to get the working tmpdir returned so that we could store additional certs (such as service account public and private keys and kubeconf for this apiserver) if needed, which will be cleaned up automatically by the teardownFn when the api server is terminating.

Release note:

NONE

ref: #50144

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 12, 2017
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 12, 2017
@dhilipkumars
Copy link
Author

cc: @sttts

@dhilipkumars
Copy link
Author

/sig testing

@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Nov 12, 2017
@dhilipkumars
Copy link
Author

/test pull-kubernetes-unit

func StartTestServer(t *testing.T) (result *restclient.Config, tearDownForCaller TearDownFunc, err error) {
var tmpDir string
//
func StartTestServer(t *testing.T, saPubKeyPath string, plugins []string, insecure bool) (result *restclient.Config, tearDownForCaller TearDownFunc, tmpDir string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make a TestServer struct out of all the return values?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please rebase onto #51685. I added a flags []string to control the config. I don't want to get any more logic in here in this func. Flags enforce that we mostly have an e2e setup as with a real kube-apiserver.

Copy link
Author

Choose a reason for hiding this comment

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

Okay i guess you mean i wait for the PR to be merged? before continuing to work on this? If i rebase now wont this PR also get all the commits #51685 as its still not in the master yet? (Apologies for the silly question i have not rebased from a PR to a PR before, just confused)

How about a struct with methods?

type TestServer struct {
         //All the return values
}

func (T *TestServer) Start(....)....
func (T *TestServer) StartTestServerOrDie(...)...
.
.
.

Okay?

If yes , I will update this PR to only make this pkg from set of functions to struct with methods along with updating all its invocations.

Copy link
Contributor

Choose a reason for hiding this comment

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

just the return values, not the inputs, i.e. also no methods.

IMO we should not add anything to the inputs anymore, but use the flags instead (as done in my PR). Everything else results in non-standard apiserver instances again which this whole machanism was trying to fix (compared to the situation we have in integration tests).

@dhilipkumars
Copy link
Author

/test pull-kubernetes-unit

@ncdc
Copy link
Member

ncdc commented Nov 14, 2017

/unassign
/assign @sttts

@k8s-ci-robot k8s-ci-robot assigned sttts and unassigned ncdc Nov 14, 2017
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 16, 2017
@dhilipkumars
Copy link
Author

dhilipkumars commented Nov 16, 2017

@sttts I have moved commits from #51685 to this one, i will have to apply your suggestions with respect to return values, right now trying to fix the CI.

@dhilipkumars
Copy link
Author

FYI : @spiffxp , @dims

@dhilipkumars
Copy link
Author

/test pull-kubernetes-unit

@dhilipkumars
Copy link
Author

/test pull-kubernetes-unit

@dhilipkumars
Copy link
Author

dhilipkumars commented Nov 17, 2017

This test repetitively fails in spite of this PR not doing anything specific to scale api object, it also appears that this test takes more than 10 mins (600 sec ) in which case go test will bring it down and mark as failed.

I1116 15:27:33.032] FAIL	k8s.io/kubernetes/test/integration/scale	600.384s

more evidence

panic: test timed out after 10m0s

goroutine 2887 [running]:
testing.startAlarm.func1()
        /home/d/goroot/go/src/testing/testing.go:1145 +0xf9
created by time.goFunc
        /home/d/goroot/go/src/time/sleep.go:170 +0x44

goroutine 1 [chan receive, 10 minutes]:
testing.(*T).Run(0xc420baa000, 0x3dcc1a3, 0x15, 0x3ed9878, 0x485636)
        /home/d/goroot/go/src/testing/testing.go:790 +0x2fc
testing.runTests.func1(0xc420baa000)
        /home/d/goroot/go/src/testing/testing.go:1004 +0x64
testing.tRunner(0xc420baa000, 0xc420c91de0)
        /home/d/goroot/go/src/testing/testing.go:746 +0xd0
testing.runTests(0xc4202fc880, 0x9116220, 0x1, 0x1, 0x30)
        /home/d/goroot/go/src/testing/testing.go:1002 +0x2d8
testing.(*M).Run(0xc4225f1f18, 0xc420c91f70)
        /home/d/goroot/go/src/testing/testing.go:921 +0x111
main.main()
        k8s.io/kubernetes/test/integration/scale/_test/_testmain.go:44 +0xdb

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2017
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 22, 2017
…ests

abstract out etcd server creation
test/integration/framework: cleanup master_utils.go
kube-apiserver: move StartTestServer tests into test/integration/master
Fix the failing scale test
kube-apiserver's TestServer now returns a struct instead of individual values
@dhilipkumars
Copy link
Author

dhilipkumars commented Nov 22, 2017

/priority important-soon

/kind feature.

@k8s-ci-robot k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 22, 2017
@dhilipkumars
Copy link
Author

@sttts could you LGTM again please? i had to rebase.

@dhilipkumars
Copy link
Author

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 22, 2017
@sttts
Copy link
Contributor

sttts commented Nov 22, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhilipkumars, dims, sttts

Associated issue requirement bypassed by: sttts

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@dhilipkumars
Copy link
Author

dhilipkumars commented Nov 22, 2017

@kubernetes/kubernetes-milestone-maintainers could someone say the below please
/status approved-for-milestone

@k8s-ci-robot
Copy link
Contributor

You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to add status labels.

@spiffxp
Copy link
Member

spiffxp commented Nov 22, 2017

/status approved-for-milestone

@spiffxp
Copy link
Member

spiffxp commented Nov 22, 2017

/status in-progress

@jberkus
Copy link

jberkus commented Nov 22, 2017

/priority critical-urgent

/remove-priority important-soon

adjusting priorities for code freeze

@k8s-ci-robot k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 22, 2017
@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Current

@dhilipkumars @mikedanese @sttts

Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required
Pull Request Labels
  • sig/testing: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/feature: New functionality.
Help

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 55545, 55548, 55815, 56136, 56185). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 704a0b5 into kubernetes:master Nov 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants