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

Apply mc-name-suffix annotation to kubeletconfig #3353

Closed
wants to merge 3 commits into from
Closed

Apply mc-name-suffix annotation to kubeletconfig #3353

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae87b31
Apply mc-name-suffix annotation to kubeletconfig
SrinivasAtmakuri Jan 15, 2024
baecdcc
updates
hawkowl Jan 18, 2024
9af15a0
revert systemreserved changes
hawkowl Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions pkg/operator/controllers/autosizednodes/autosizednodes_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ type Reconciler struct {
const (
ControllerName = "AutoSizedNodes"
configName = "dynamic-node"

// machineconfiguration annotation added on the kubelet CRD - dynamic-node
mcAnnotationName = "machineconfiguration.openshift.io/mc-name-suffix"
mcAnnotationValue = ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would comment why we use "" here just so we remember in a while, it's unintuitive.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would comment why we use "" here just so we remember in a while, it's unintuitive.

Or we can make the name descriptive (e.g. lowPriority)?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's because the value "" is viewed by MCO as zero. Hm.

)

func NewReconciler(log *logrus.Entry, client client.Client) *Reconciler {
Expand Down Expand Up @@ -90,7 +94,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
return ctrl.Result{}, fmt.Errorf("could not fetch KubeletConfig: %w", err)
}

// If already exists, update the spec
// If already exists, update the spec and annotations
if config.Annotations == nil {
lranjbar marked this conversation as resolved.
Show resolved Hide resolved
config.Annotations = defaultConfig.Annotations
}
if val, ok := config.Annotations[mcAnnotationName]; !ok || val != mcAnnotationValue {
Copy link
Member

Choose a reason for hiding this comment

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

If this is already set, we shouldn't be changing it regardless of value, as it will disconnect the kubeletconfig from the already rendered machineconfig

Copy link
Contributor

Choose a reason for hiding this comment

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

This is true, we understand MCO behaviour in this condition.

config.Annotations[mcAnnotationName] = mcAnnotationValue
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if a different kubeletconfig (maybe one provided by the customer) already his this annotation and value?

Copy link
Contributor

Choose a reason for hiding this comment

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

Srini did some testing on this and found that if two kubeletconfig have the same value for this annotation it breaks MCO. This is potentially a breaking issue. I think we should rethink this approach because of this

}

config.Spec = defaultConfig.Spec
err = r.client.Update(ctx, &config)
if err != nil {
Expand Down Expand Up @@ -120,7 +131,8 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
func makeConfig() mcv1.KubeletConfig {
return mcv1.KubeletConfig{
ObjectMeta: metav1.ObjectMeta{
Name: configName,
Name: configName,
Annotations: map[string]string{mcAnnotationName: mcAnnotationValue},
},
Spec: mcv1.KubeletConfigSpec{
AutoSizingReserved: to.BoolPtr(true),
Expand Down
6 changes: 5 additions & 1 deletion pkg/operator/controllers/autosizednodes/autosizednodes_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ func TestAutosizednodesReconciler(t *testing.T) {
aro(true),
&mcv1.KubeletConfig{
ObjectMeta: metav1.ObjectMeta{
Name: configName,
Name: configName,
Annotations: map[string]string{"machineconfiguration.openshift.io/mc-name-suffix": "2"},
},
Spec: mcv1.KubeletConfigSpec{
AutoSizingReserved: to.BoolPtr(false),
Expand Down Expand Up @@ -121,6 +122,9 @@ func TestAutosizednodesReconciler(t *testing.T) {
if !reflect.DeepEqual(test.wantConfig.Spec, c.Spec) {
t.Error(cmp.Diff(test.wantConfig.Spec, c.Spec))
}
if !reflect.DeepEqual(test.wantConfig.Annotations, c.Annotations) {
t.Error(cmp.Diff(test.wantConfig.Annotations, c.Annotations))
}

if result != (ctrl.Result{}) {
t.Error("reconcile returned an unexpected result")
Expand Down
4 changes: 4 additions & 0 deletions pkg/operator/controllers/workaround/const.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ const (
workerMachineConfigPoolName = "worker"
memReserved = "2000Mi"

// machineconfiguration annotation added on the kubelet CRD - aro-limits
mcAnnotationName = "machineconfiguration.openshift.io/mc-name-suffix"
lranjbar marked this conversation as resolved.
Show resolved Hide resolved
mcAnnotationValue = ""

// ifreload workaround
kubeNamespace = "openshift-azure-ifreload"
)
4 changes: 4 additions & 0 deletions pkg/operator/controllers/workaround/systemreserved.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ func (sr *systemreserved) Ensure(ctx context.Context) error {
kc.Labels = make(map[string]string)
}
kc.Labels[labelName] = labelValue
if kc.Annotations == nil {
lranjbar marked this conversation as resolved.
Show resolved Hide resolved
kc.Annotations = make(map[string]string)
}
kc.Annotations[mcAnnotationName] = mcAnnotationValue

b, err := json.Marshal(map[string]interface{}{
"systemReserved": map[string]interface{}{
Expand Down
3 changes: 3 additions & 0 deletions pkg/operator/controllers/workaround/systemreserved_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ func TestSystemreservedEnsure(t *testing.T) {
Labels: map[string]string{
"aro.openshift.io/limits": "",
},
Annotations: map[string]string{
"machineconfiguration.openshift.io/mc-name-suffix": "",
},
ResourceVersion: resourceVersion,
},
Spec: mcv1.KubeletConfigSpec{
Expand Down
Loading