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

Change node finalizer name to match api requirements #84

Merged
merged 1 commit into from Jul 10, 2020

Conversation

n1r1
Copy link

@n1r1 n1r1 commented Jul 9, 2020

It seems that k8s doesn't accept finalizer name if it doesn't contain /
See this for more details:
https://github.com/kubernetes/kubernetes/blob/205d5c58296357365bfa834ecabb384c1c1042c3/pkg/apis/core/validation/validation.go#L5515

Also, I got an error after adding trailing / without any value

Fixing https://bugzilla.redhat.com/show_bug.cgi?id=1855049
The error message:

020/07/08 18:12:51 Failed to add node finalizer to node worker-0-0, error: Node "worker-0-0" is invalid: metadata.finalizers[0]: Invalid value: "capbm.metal3.io": name is neither a standard finalizer name nor is it fully qualified     
E0708 18:12:51.059738       1 controller.go:275] ocp-edge-cluster-rdu2-0-worker-0-p722p: error updating machine: Node "worker-0-0" is invalid: metadata.finalizers[0]: Invalid value: "capbm.metal3.io": name is neither a standard finalizer name nor is it fully qualified
{"level":"error","ts":1594231971.0597603,"logger":"controller-runtime.controller","msg":"Reconciler error","controller":"machine_controller","request":"openshift-machine-api/ocp-edge-cluster-rdu2-0-worker-0-p722p","error":"Node \"worker-0-0\" is invalid: metadata.finalizers[0]: Invalid value: \"capbm.metal3.io\": name is neither a standard finalizer name nor is it fully qualified","stacktrace":"github.com/go-logr/zapr.(*zapLogger).Error\n\t/go/src/github.com/openshift/cluster-api-provider-baremetal/vendor/github.com/go-logr/zapr/zapr.go:128\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/go/src/github.com/openshift/cluster-api-provider-baremetal/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:258\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/go/src/github.com/openshift/cluster-api-provider-baremetal/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:232\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).worker\n\t/go/src/github.com/openshift/cluster-api-provider-baremetal/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:211\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1\n\t/go/src/github.com/openshift/cluster-api-provider-baremetal/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:155\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil\n\t/go/src/github.com/openshift/cluster-api-provider-baremetal/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:156\nk8s.io/apimachinery/pkg/util/wait.JitterUntil\n\t/go/src/github.com/openshift/cluster-api-provider-baremetal/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:133\nk8s.io/apimachinery/pkg/util/wait.Until\n\t/go/src/github.com/openshift/cluster-api-provider-baremetal/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:90"}     

It seems that k8s doesn't accpet finalizer name if it doesn't contain /
See this for more details:
https://github.com/kubernetes/kubernetes/blob/205d5c58296357365bfa834ecabb384c1c1042c3/pkg/apis/core/validation/validation.go#L5515

Also, I got an error after adding trailing / without any value

Fixing https://bugzilla.redhat.com/show_bug.cgi?id=1855049

Signed-off-by: Nir <niry@redhat.com>
@n1r1 n1r1 changed the title Add trailing / to node finalizer name Change node finalizer name to match api requirements Jul 9, 2020
@dhellmann
Copy link

Have we already released a version with the old finalizer name? If so, what's going to happen to resources that had the old finalizer name applied?

@n1r1
Copy link
Author

n1r1 commented Jul 9, 2020

No. It was introduced in 4.6

@dhellmann
Copy link

/approve

/cc @hardys

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2020
@hardys
Copy link

hardys commented Jul 10, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, hardys, n1r1

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

@openshift-merge-robot openshift-merge-robot merged commit cb7f02a into openshift:master Jul 10, 2020
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants