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

OCPBUGS-5825: Removes legacy cloud-provider resources #778

Merged
merged 1 commit into from Dec 14, 2023
Merged

OCPBUGS-5825: Removes legacy cloud-provider resources #778

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 15 additions & 2 deletions pkg/operator/starter.go
View file
Open in desktop
Expand Up @@ -138,8 +138,6 @@ func RunOperator(ctx context.Context, cc *controllercmd.ControllerContext) error
"assets/kube-controller-manager/localhost-recovery-token.yaml",
"assets/kube-controller-manager/csr_approver_clusterrole.yaml",
"assets/kube-controller-manager/csr_approver_clusterrolebinding.yaml",
"assets/kube-controller-manager/gce/cloud-provider-role.yaml",
"assets/kube-controller-manager/gce/cloud-provider-binding.yaml",
},
(&resourceapply.ClientHolder{}).WithKubernetes(kubeClient),
operatorClient,
Expand All @@ -165,6 +163,21 @@ func RunOperator(ctx context.Context, cc *controllercmd.ControllerContext) error
return isVSphere
},
nil,
).WithConditionalResources(
bindata.Asset,
[]string{
"assets/kube-controller-manager/gce/cloud-provider-role.yaml",
Copy link
Member

@atiratree atiratree Dec 14, 2023 •
edited

Choose a reason for hiding this comment

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

please remove these two files from the main list above

"assets/kube-controller-manager/gce/cloud-provider-binding.yaml",
},
func() bool {
// We do not want to apply these resources, so must return false here
return false
},
func() bool {
// The resources above are required for the 4.14 -> 4.15 upgrade path.
Copy link
Member

Choose a reason for hiding this comment

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

are you planning to backport this to 4.15 branch?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this would break 4.14 to 4.15 upgrades, that's why we are doing this now. The file needs to be removed only once the upgrade to 4.15 is complete, it cannot be removed mid update as that could cause KCM to fall over and halt the upgrade.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, 4.15 is not released yet, so I think it could still work if we do it before the release.

Copy link
Member

Choose a reason for hiding this comment

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

^ if the RBAC is not used in the 4.14

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been working on the basis that RBAC is used in 4.14, that's why we are removing now, but I will double check when we stopped using it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, confirmed, RBAC is used in 4.14, not used in 4.15, so I want to remove only from 4.16 to avoid breaking upgrades

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for looking into this. Ok let's remove this in 4.16 then.

// They are not required from 4.16, so can re removed.
return true
},
).AddKubeInformers(kubeInformersForNamespaces)

targetConfigController := targetconfigcontroller.NewTargetConfigController(
Expand Down