Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

Update HCR & HNSR to move NS creation to HNSR #550

Merged

Conversation

yiqigao217
Copy link
Contributor

@yiqigao217 yiqigao217 commented Mar 24, 2020

This commit removes the hc.Spec.RequiredChildren field and the
enable-hns-reconciler flag. It also moves the creation of the owned
(previously requiredChild) namespaces from HC Reconciler (HCR) to HNS
Reconciler (HNSR). The logic in HCR is cleaned up to adapt this change.

Tested with the current and updated integration tests by "make test".
Also manually tested the 'k hns describe'. The HNS_MISSING condition was
set when I deleted the HNS instance in the owner. The owned namespace
got recreated after I deleted it, which means the new watch in HNSR is
working.

Fully described in #552
Part of #457
Fixes #479

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 24, 2020
@k8s-ci-robot k8s-ci-robot requested review from Fei-Guo and rjbez17 March 24, 2020 21:53
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 24, 2020
@yiqigao217 yiqigao217 force-pushed the cascadingDelete-HNSR branch from 31b8416 to 38283a6 Compare March 24, 2020 21:58
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2020
@yiqigao217
Copy link
Contributor Author

/assign @adrianludwin
/assign @sophieliu15

Copy link
Contributor

@adrianludwin adrianludwin left a comment

Choose a reason for hiding this comment

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

Great work so far. I left a lot of comments because the more I think about this, the more I think we can make it even simpler. LMK when you want to discuss.

incubator/hnc/pkg/kubectl/describe.go Outdated Show resolved Hide resolved
incubator/hnc/pkg/reconcilers/hierarchical_namespace.go Outdated Show resolved Hide resolved
incubator/hnc/pkg/reconcilers/hierarchical_namespace.go Outdated Show resolved Hide resolved
incubator/hnc/pkg/reconcilers/hierarchical_namespace.go Outdated Show resolved Hide resolved
incubator/hnc/pkg/reconcilers/hierarchy_config.go Outdated Show resolved Hide resolved
incubator/hnc/pkg/reconcilers/hierarchy_config.go Outdated Show resolved Hide resolved
incubator/hnc/pkg/reconcilers/hierarchy_config.go Outdated Show resolved Hide resolved
@yiqigao217 yiqigao217 force-pushed the cascadingDelete-HNSR branch from 73f2c03 to 09093f8 Compare March 26, 2020 14:23
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 26, 2020
@yiqigao217 yiqigao217 force-pushed the cascadingDelete-HNSR branch 2 times, most recently from 0a52325 to 133aa65 Compare March 26, 2020 15:23
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 26, 2020
@yiqigao217 yiqigao217 changed the title Update HC Spec and HCR to move NS creation to HNSR Update HCR & HNSR to move NS creation to HNSR Mar 26, 2020
Copy link
Contributor

@adrianludwin adrianludwin left a comment

Choose a reason for hiding this comment

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

I like this a lot :)

incubator/hnc/pkg/kubectl/describe.go Outdated Show resolved Hide resolved
incubator/hnc/api/v1alpha1/hierarchical_namespace.go Outdated Show resolved Hide resolved
incubator/hnc/pkg/reconcilers/hierarchy_config.go Outdated Show resolved Hide resolved
incubator/hnc/pkg/reconcilers/hierarchy_config.go Outdated Show resolved Hide resolved
incubator/hnc/pkg/reconcilers/hierarchy_config.go Outdated Show resolved Hide resolved
incubator/hnc/pkg/validators/hierarchy.go Outdated Show resolved Hide resolved
This commit removes the hc.Spec.RequiredChildren field and the
enable-hns-reconciler flag. It also moves the creation of the owned
(previously requiredChild) namespaces from HC Reconciler (HCR) to HNS
Reconciler (HNSR). The logic in HCR is cleaned up to adapt this change.

Tested with the current and updated integration tests by "make test".
Also manually tested the 'k hns describe'. The HNS_MISSING condition was
set when I deleted the HNS instance in the owner. The owned namespace
got recreated after I deleted it, which means the new watch in HNSR is
working.
@yiqigao217 yiqigao217 force-pushed the cascadingDelete-HNSR branch from 133aa65 to b287126 Compare March 26, 2020 18:37
@adrianludwin
Copy link
Contributor

/lgtm
/approve
/hold
/assign @rjbez17

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 26, 2020
@k8s-ci-robot k8s-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 26, 2020
@rjbez17
Copy link

rjbez17 commented Mar 26, 2020

/lgtm
/approve
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 26, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianludwin, rjbez17, yiqigao217

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:
  • OWNERS [adrianludwin,rjbez17]

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 merged commit 8f6d3eb into kubernetes-retired:master Mar 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HNC: Clean up before setting HNS reconciler as default
5 participants