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.

Sign up for GitHub

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

Jump to bottom

Adding cadcading deletion support for federated secrets #36296

Merged
merged 2 commits into from
Nov 8, 2016
Merged

Adding cadcading deletion support for federated secrets #36296

merged 2 commits into from
Nov 8, 2016

Conversation

nikhiljindal
Copy link
Contributor

@nikhiljindal nikhiljindal commented Nov 6, 2016 •
edited
Loading

Ref #33612

Adding cascading deletion support for federated secrets.
The code is same as that for namespaces. Just ensuring that DeletionHelper functions are called at right places in secret_controller.
Also added e2e tests.

cc @kubernetes/sig-cluster-federation @caesarxuchao

federation: Adding support for DeleteOptions.OrphanDependents for federated secrets. Setting it to false while deleting a federated secret also deletes the corresponding secrets from all registered clusters.

This change is Reviewable

@nikhiljindal nikhiljindal added this to the v1.5 milestone Nov 6, 2016
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Nov 6, 2016
@nikhiljindal nikhiljindal added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Nov 6, 2016
caesarxuchao
caesarxuchao reviewed Nov 6, 2016
View reviewed changes
federation/pkg/federation-controller/secret/secret_controller.go
baseSecret.Name)
// Add the required finalizers before creating a secret in
// underlying clusters.
// This ensures that the dependent secrets are deleted in underlying
Copy link
Member

Choose a reason for hiding this comment

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

Update the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

caesarxuchao
caesarxuchao reviewed Nov 6, 2016
View reviewed changes
federation/pkg/federation-controller/secret/secret_controller.go
// delete deletes the given secret or returns error if the deletion was not complete.
func (secretcontroller *SecretController) delete(secret *api_v1.Secret) error {
glog.V(3).Infof("Handling deletion of secret: %v", *secret)
// Delete the secret from all underlying clusters.
Copy link
Member

Choose a reason for hiding this comment

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

Update the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it

caesarxuchao
caesarxuchao reviewed Nov 6, 2016
View reviewed changes
federation/pkg/federation-controller/secret/secret_controller_test.go
@@ -92,6 +95,11 @@ func TestSecretController(t *testing.T) {

// Test add federated secret.
secretWatch.Add(&secret1)
updatedSecret := GetSecretFromChan(secretUpdateChan)
assert.True(t, secretController.hasFinalizerFunc(updatedSecret, deletionhelper.FinalizerDeleteFromUnderlyingClusters))
Copy link
Member

Choose a reason for hiding this comment

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

Check for both finalizers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@caesarxuchao
Copy link
Member

LGTM. Update the comments, and update the assert in the e2e test. Feel free to self apply the label.

@nikhiljindal
Copy link
Contributor Author

Updated the code as per comments.
Thanks a lot @caesarxuchao for the review!

@nikhiljindal nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2016
@nikhiljindal
Copy link
Contributor Author

@k8s-bot unit test this

1 similar comment
@nikhiljindal
Copy link
Contributor Author

@k8s-bot unit test this

@nikhiljindal nikhiljindal added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 7, 2016
@nikhiljindal nikhiljindal added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 7, 2016
@nikhiljindal
Copy link
Contributor Author

@k8s-bot unit test this

1 similar comment
@nikhiljindal
Copy link
Contributor Author

@k8s-bot unit test this

@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 7, 2016
@nikhiljindal
Copy link
Contributor Author

Rebased

@nikhiljindal nikhiljindal removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2016
@nikhiljindal nikhiljindal added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 7, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit ed1b037402ade81aa84fa0322d1b537cb94e7949. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit ed1b037402ade81aa84fa0322d1b537cb94e7949. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit ed1b037402ade81aa84fa0322d1b537cb94e7949. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit ed1b037402ade81aa84fa0322d1b537cb94e7949. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit ed1b037402ade81aa84fa0322d1b537cb94e7949. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit ed1b037402ade81aa84fa0322d1b537cb94e7949. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit ed1b037402ade81aa84fa0322d1b537cb94e7949. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 9cda48e4f9781d6ed7647e0a42e3586763cca9e0. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@nikhiljindal nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2016
@nikhiljindal nikhiljindal mentioned this pull request Nov 7, 2016
Merged
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@saad-ali
Copy link
Member

saad-ali commented Nov 8, 2016

Release-czar approved post-code freeze merge--This was LGTMed and in the merge-queue at code freeze time for 1.5. Leaving 1.5 milestone to let it gets merged after code freeze.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit d8fa6a9 into kubernetes:master Nov 8, 2016
@rmmh
Copy link
Contributor

rmmh commented Nov 8, 2016

@nikhiljindal @pwittrock this is making test-go fail ~25% of the time (#36422). Can this be reverted if it's not an easy fix?

@nikhiljindal
Copy link
Contributor Author

Sorry just saw #36422. Am on it

@chentao1596 chentao1596 mentioned this pull request Dec 5, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caesarxuchao caesarxuchao caesarxuchao left review comments

Assignees

@caesarxuchao caesarxuchao

Labels
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@nikhiljindal @caesarxuchao @k8s-ci-robot @k8s-github-robot @saad-ali @rmmh @madhusudancs @googlebot