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

Add Garbage Collector e2e conformance tests #60116

Merged
merged 1 commit into from
Feb 27, 2018

Conversation

jennybuckley
Copy link

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 21, 2018
@k8s-ci-robot k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Feb 21, 2018
@lavalamp
Copy link
Member

/assign @caesarxuchao

cc @wenjiaswe

@jennybuckley
Copy link
Author

@kubernetes/sig-architecture-pr-reviews

@bgrant0607 bgrant0607 self-assigned this Feb 23, 2018
@bgrant0607
Copy link
Member

@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
Copy link
Member

cc @smarterclayton

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. 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,
Copy link
Member

Choose a reason for hiding this comment

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

s/false/true/
?

Copy link
Member

@caesarxuchao caesarxuchao left a comment

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

Please use DeletePropagationOrphan policy.

@bgrant0607
Copy link
Member

Tests using deprecated features should not be added to conformance.

@bgrant0607
Copy link
Member

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

Conformance should test GA, non-deprecated features.

@jennybuckley
Copy link
Author

/test pull-kubernetes-verify

@jennybuckley
Copy link
Author

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
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2018
@caesarxuchao
Copy link
Member

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
Copy link
Member

FYI, some tests failed.

/approve

@k8s-ci-robot
Copy link
Contributor

[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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2018
@bgrant0607
Copy link
Member

#60442 just merged

/test pull-kubernetes-verify

@jennybuckley
Copy link
Author

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

@k8s-github-robot
Copy link

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-github-robot k8s-github-robot merged commit 6546b69 into kubernetes:master Feb 27, 2018
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants