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

Register RuntimeClass CRD in nodelifecycle controller #67924

Closed
wants to merge 1 commit into from

Conversation

tallclair
Copy link
Member

What this PR does / why we need it:

Register the RuntimeClass CRD when the RuntimeClass feature gate is enabled. This is done in the node lifecycle controller as a convenient peripherally related component.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
For kubernetes/enhancements#585

Release note:
Covered by #67737

NONE

/sig node
/kind feature
/priority important-soon
/milestone v1.12

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Aug 28, 2018
@k8s-ci-robot k8s-ci-robot added this to the v1.12 milestone Aug 28, 2018
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 28, 2018
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 28, 2018
@tallclair
Copy link
Member Author

/assign @mikedanese @caesarxuchao
/cc @yujuhong @dchen1107

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tallclair
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mikedanese

If they are not already assigned, you can assign the PR to them by writing /assign @mikedanese in a comment when ready.

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 sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Aug 29, 2018
@tallclair
Copy link
Member Author

/retest

@caesarxuchao
Copy link
Member

The code is fine, but I got a few high-level questions.

  • Why does this logic belong to node-lifecycle controller?
  • If the CRD is accidentally deleted, will it stop pod scheduling? Shall we set up rbac rules to make it difficult for users to shoot in the foot?
  • If the cluster has existing RuntimeClass CRD, shall the controller update it the CRD? For example, suppose 1.13 RuntimeClass has a v1beta1 version, and a cluster is downgraded from 1.13 to 1.12, the 1.12 controller should update the CRD definition to remove v1beta1 from the version list. But then if it's an HA-cluster, the final state of the CRD is indeterminable during upgrade/downgrade... Maybe instead of installing the CRD during startup, the controller needs install the CRD in a sync loop.

@tallclair
Copy link
Member Author

Why does this logic belong to node-lifecycle controller?

It doesn't, particularly. I chose the node-lifecycle controller as the closest fit from the existing controllers. It didn't seem worth adding a new controller component just to install the CRD. We may introduce a control-plane component in the future, in which case it would make sense to move the CRD registration to that component instead. WDYT?

If the CRD is accidentally deleted, will it stop pod scheduling?

What happens to existing resources if the CRD is deleted? What happens to the instances cached in informers? A pod that doesn't reference a RuntimeClass (or the empty "" runtime class) is always allowed. In general, I'm OK with this for alpha, but it's something to think about for beta.

Shall we set up rbac rules to make it difficult for users to shoot in the foot?

RBAC rules are additive / allow only, so I don't think there's anything we can do there. There is a broader open question around how we want to deal with this for core components. Again, I think for alpha, it's OK to ignore it.

If the cluster has existing RuntimeClass CRD, shall the controller update it the CRD? For example, suppose 1.13 RuntimeClass has a v1beta1 version, and a cluster is downgraded from 1.13 to 1.12, the 1.12 controller should update the CRD definition to remove v1beta1 from the version list. But then if it's an HA-cluster, the final state of the CRD is indeterminable during upgrade/downgrade... Maybe instead of installing the CRD during startup, the controller needs install the CRD in a sync loop.

Interesting. What happens to the existing instances if the the schema changes? Since it's a CRD, my hunch is that leaving it in place will be preferable, and it will just be ignored by the core components at that point, but I'll defer to the experts. I'll follow up on this.

@tallclair
Copy link
Member Author

After investigating, I think doing this through an addon is preferable. See the alternative approach in #68161

@tallclair tallclair closed this Sep 1, 2018
k8s-github-robot pushed a commit that referenced this pull request Sep 5, 2018
Automatic merge from submit-queue (batch tested with PRs 68161, 68023, 67909, 67955, 67731). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

Register RuntimeClass CRD as an addon

**What this PR does / why we need it**:

Register the RuntimeClass CRD when the RuntimeClass feature gate is enabled. This is done in through the addon manager.

This is an alternative approach to #67924

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
For kubernetes/enhancements#585

**Release note**:
Covered by #67737
```release-note
NONE
```

/sig node
/kind feature
/priority important-soon
/milestone v1.12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/node Categorizes an issue or PR as relevant to SIG Node. 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

4 participants