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

Add Garbage Collector e2e conformance tests #60116

Merged
merged 1 commit into from Feb 27, 2018

Conversation

@jennybuckley
Contributor

jennybuckley commented Feb 21, 2018

What this PR does / why we need it:
The garbage collector is a core component of kubernetes and needs to be tested by conformance, so its functionality can be relied on in any kubernetes environment.

As we can see in testgrid, the garbage collector tests being promoted by this PR are consistently passing. And the intention to promote them to conformance tests was laid out by this document

Special notes for your reviewer:
The last two tests in this file are not added as conformance tests because they involve beta features (custom resources and cronjobs), and conformance tests are only allowed for features in GA.

Release note:

New conformance tests added for the Garbage Collector
@lavalamp

This comment has been minimized.

Member

lavalamp commented Feb 22, 2018

@jennybuckley

This comment has been minimized.

Contributor

jennybuckley commented Feb 23, 2018

@bgrant0607 bgrant0607 self-assigned this Feb 23, 2018

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented Feb 26, 2018

@jennybuckley It's reasonable to add these tests to the conformance suite, since the functionality is now GA. However, the tests need names and descriptions in comments. Search the repo for "ConformanceIt" for examples.

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented Feb 26, 2018

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Feb 26, 2018

It("should orphan RS created by deployment when deleteOptions.OrphanDependents is true", func() {
/*
Testname: garbage-collector-delete-deployment-orphaning-true
Description: Ensure that if deleteOptions.OrphanDependents is set to false,

This comment has been minimized.

@bgrant0607

bgrant0607 Feb 26, 2018

Member

s/false/true/
?

@caesarxuchao

We need to update the tests to use the DeleteOptions.DeletionPropagation instead of the OrphanDependents field, which is deprecated. Otherwise lgtm.

/*
Testname: garbage-collector-delete-rc-orphaning-false
Description: Ensure that if deleteOptions.OrphanDependents is set to false,

This comment has been minimized.

@caesarxuchao

caesarxuchao Feb 26, 2018

Member

OrphanDependents is deprecated. We should update the test to use deleteOptions.PropagationPolicy (https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L455). Could you update the test to use the getBackgroundOptions() instead of getNonOrphanOptions()? That's just an API change, shouldn't cause any behavior change.

It("should keep the rc around until all its pods are deleted if the deleteOptions says so", func() {
/*
Testname: garbage-collector-delete-rc-after-owned-pods
Description: Ensure that if deleteOptions.OrphanDependents is set to false,

This comment has been minimized.

@caesarxuchao

caesarxuchao Feb 26, 2018

Member

Ensure that if deleteOptions.propagationPolicy="Foreground", ...

It("should not delete dependents that have both valid owner and owner that's waiting for dependents to be deleted", func() {
/*
Testname: garbage-collector-multiple-owners
Description: Ensure that if deleteOptions.PropogationPolicy is set to

This comment has been minimized.

@caesarxuchao

caesarxuchao Feb 26, 2018

Member

Could you update the description to match the description in "ConformanceIt"?

It("should orphan pods created by rc if delete options say so", func() {
/*
Testname: garbage-collector-delete-rc-orphaning-true
Description: Ensure that if deleteOptions.OrphanDependents is set to true,

This comment has been minimized.

@caesarxuchao

caesarxuchao Feb 26, 2018

Member

ditto. Please update the test to use DeletePropagationOrphan.

It("should orphan pods created by rc if deleteOptions.OrphanDependents is nil", func() {
/*
Testname: garbage-collector-delete-rc-orphaning-unset
Description: Ensure that if deleteOptions.OrphanDependents is set to nil,

This comment has been minimized.

@caesarxuchao

caesarxuchao Feb 26, 2018

Member

This just requires a change in the comment. Both OrphanDependents and PropagationPolicy are default to nil.

It("should delete RS created by deployment when not orphaning", func() {
/*
Testname: garbage-collector-delete-deployment-orphaning-false
Description: Ensure that if deleteOptions.OrphanDependents is set to false,

This comment has been minimized.

@caesarxuchao

caesarxuchao Feb 26, 2018

Member

Please update to DeletePropagationBackground.

It("should orphan RS created by deployment when deleteOptions.OrphanDependents is true", func() {
/*
Testname: garbage-collector-delete-deployment-orphaning-true
Description: Ensure that if deleteOptions.OrphanDependents is set to false,

This comment has been minimized.

@caesarxuchao

caesarxuchao Feb 26, 2018

Member

Please use DeletePropagationOrphan policy.

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented Feb 26, 2018

Tests using deprecated features should not be added to conformance.

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented Feb 26, 2018

Thanks. I'll wait for @caesarxuchao to re-review before approving.

Conformance should test GA, non-deprecated features.

@jennybuckley

This comment has been minimized.

Contributor

jennybuckley commented Feb 26, 2018

/test pull-kubernetes-verify

@jennybuckley

This comment has been minimized.

Contributor

jennybuckley commented Feb 26, 2018

I think https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/60116/pull-kubernetes-verify/79482/ is not a legitimate failure. All PRs seem to be failing the same way

@caesarxuchao

This comment has been minimized.

Member

caesarxuchao commented Feb 26, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 26, 2018

@caesarxuchao

This comment has been minimized.

Member

caesarxuchao commented Feb 26, 2018

All the tested features here have been stable for more than 1 year. GC of custom resources have flakeness in tests but they are not tested here.

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented Feb 26, 2018

FYI, some tests failed.

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 26, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bgrant0607, caesarxuchao, jennybuckley

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented Feb 26, 2018

#60442 just merged

/test pull-kubernetes-verify

@jennybuckley

This comment has been minimized.

Contributor

jennybuckley commented Feb 27, 2018

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 27, 2018

Automatic merge from submit-queue (batch tested with PRs 60430, 60115, 58052, 60355, 60116). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 6546b69 into kubernetes:master Feb 27, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation jennybuckley authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment