Skip to content

Upgrade from AppWrapper v1beta1 to v1beta2 #491

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

Closed
wants to merge 26 commits into from
Closed

Upgrade from AppWrapper v1beta1 to v1beta2 #491

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
a7a9bca
Upgrade to AppWrapper v1beta2
dgrove-oss Mar 19, 2024
635119b
Disable operator upgrade test for MCAD to Kueue transition
dgrove-oss Mar 25, 2024
5e3595f
update for review changes in codeflare-common
dgrove-oss Mar 28, 2024
c3fa25d
review comments
dgrove-oss Mar 28, 2024
5d907b5
pick up codeflare-common@mcadv1b2 changes
dgrove-oss Mar 28, 2024
04b9b1f
port to AppWrapper controller using cert-controller
dgrove-oss Mar 28, 2024
c8ead36
Kueue-enable e2e test cases
dgrove-oss Mar 29, 2024
f291e02
RayCluster test with AppWrappers
dgrove-oss Mar 29, 2024
43c8bc9
collect Kueue logs in e2e test
dgrove-oss Mar 30, 2024
f3147a6
update to appwrapper 0.7.2
dgrove-oss Apr 2, 2024
d6c87e9
validate AppWrapper config after merging delta from config map
dgrove-oss Apr 2, 2024
39c34d8
fix errors introduced in rebase
dgrove-oss Apr 3, 2024
034992d
generate AppWrapper RBAC's from kubebuilder comments
dgrove-oss Apr 3, 2024
828d0b1
appwrapper 0.7.2
dgrove-oss Apr 3, 2024
ef0665b
AppWrapper release is managed externally
dgrove-oss Apr 3, 2024
dca8dcb
tweaks
dgrove-oss Apr 3, 2024
b88ca94
allow appwrapper controller to be disabled in config
dgrove-oss Apr 4, 2024
58dcfd0
copy edit README.md
dgrove-oss Apr 4, 2024
d29c5fe
Fix type in main.go
dgrove-oss Apr 4, 2024
cbfd580
get codeflare-common from main
dgrove-oss Apr 4, 2024
b5aa4aa
review comments
dgrove-oss Apr 4, 2024
7187933
restore olm upgrade test; this test is expected to fail
dgrove-oss Apr 4, 2024
ca6b5ec
deploy Kueue in olm_tests
dgrove-oss Apr 4, 2024
2035794
review comment: keep cert config private
dgrove-oss Apr 4, 2024
e12fa53
skip AppWrapper tests when AppWrapper controller not enabled
dgrove-oss Apr 4, 2024
318f601
fix typo
dgrove-oss Apr 4, 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
Prev Previous commit
Next Next commit
allow appwrapper controller to be disabled in config
  • Loading branch information
@dgrove-oss
dgrove-oss committed Apr 4, 2024
commit b88ca9486aee36429ba710a76973e7b8572c89ee
28 changes: 17 additions & 11 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,10 @@ func main() {
QPS: ptr.To(float32(50)),
Burst: ptr.To(int32(100)),
},
AppWrapper: awconfig.NewConfig(namespace),
AppWrapper: &config.AppWrapperConfiguration{
Enabled: ptr.To(true),
Copy link
Contributor

@astefanutti astefanutti Apr 4, 2024

Choose a reason for hiding this comment

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

This should be disabled by default to start with.

Copy link
Collaborator Author

@dgrove-oss dgrove-oss Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure I can do that. It might also allow us to merge this PR without waiting on SDK changes to enable the new AppWrappers (we'd need to be sure all the old MCAD code is disabled by default in the SDK, but I think that has mostly been done already).

We should have a followup PR that adds e2e testing (another workflow?) in both this repo and the SDK that enables the AppWrapper controller though.

Copy link
Contributor

@astefanutti astefanutti Apr 4, 2024

Choose a reason for hiding this comment

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

Yes exactly, that makes things easier w.r.t. the SDK.

Copy link
Collaborator Author

@dgrove-oss dgrove-oss Apr 4, 2024

Choose a reason for hiding this comment

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

Done. I'll do a trial PR to the SDK that just makes the change to this branch of the operator but does the minimal other changes.

Config: awconfig.NewConfig(namespace),
},
ControllerManager: config.ControllerManager{
Metrics: config.MetricsConfiguration{
BindAddress: ":8080",
Expand All @@ -126,10 +129,10 @@ func main() {
RayDashboardOAuthEnabled: ptr.To(true),
},
}
cfg.AppWrapper.CertManagement.WebhookSecretName = "codeflare-operator-webhook-server-cert"
cfg.AppWrapper.CertManagement.WebhookServiceName = "codeflare-operator-webhook-service"
cfg.AppWrapper.CertManagement.MutatingWebhookConfigName = "codeflare-operator-mutating-webhook-configuration"
cfg.AppWrapper.CertManagement.ValidatingWebhookConfigName = "codeflare-operator-validating-webhook-configuration"
cfg.AppWrapper.Config.CertManagement.WebhookSecretName = "codeflare-operator-webhook-server-cert"
cfg.AppWrapper.Config.CertManagement.WebhookServiceName = "codeflare-operator-webhook-service"
cfg.AppWrapper.Config.CertManagement.MutatingWebhookConfigName = "codeflare-operator-mutating-webhook-configuration"
cfg.AppWrapper.Config.CertManagement.ValidatingWebhookConfigName = "codeflare-operator-validating-webhook-configuration"

kubeConfig, err := ctrl.GetConfig()
exitOnError(err, "unable to get client config")
Expand All @@ -140,7 +143,7 @@ func main() {
exitOnError(err, "unable to create Kubernetes client")

exitOnError(loadIntoOrCreate(ctx, kubeClient, namespaceOrDie(), configMapName, cfg), "unable to initialise configuration")
exitOnError(awconfig.ValidateConfig(cfg.AppWrapper), "invalid AppWrapper configuration")
exitOnError(awconfig.ValidateConfig(cfg.AppWrapper.Config), "invalid AppWrapper configuration")

kubeConfig.Burst = int(ptr.Deref(cfg.ClientConnection.Burst, int32(rest.DefaultBurst)))
kubeConfig.QPS = ptr.Deref(cfg.ClientConnection.QPS, rest.DefaultQPS)
Expand Down Expand Up @@ -183,7 +186,7 @@ func main() {
if os.Getenv("ENABLE_WEBHOOKS") == "false" {
close(certsReady)
} else {
exitOnError(awctrl.SetupCertManagement(mgr, &cfg.AppWrapper.CertManagement, certsReady), "unable to set up cert rotation")
exitOnError(awctrl.SetupCertManagement(mgr, &cfg.AppWrapper.Config.CertManagement, certsReady), "unable to set up cert rotation")
}

v, err := HasAPIResourceForGVK(kubeClient.DiscoveryClient, rayv1.GroupVersion.WithKind("RayCluster"))
Expand All @@ -196,12 +199,15 @@ func main() {

v1, err1 := HasAPIResourceForGVK(kubeClient.DiscoveryClient, kueue.GroupVersion.WithKind("Workload"))
v2, err2 := HasAPIResourceForGVK(kubeClient.DiscoveryClient, mcadv1beta2.GroupVersion.WithKind("AppWrapper"))
if v1 && v2 {
if v1 && v2 && *cfg.AppWrapper.Enabled {
// Ascynchronous because controllers need to wait for certificate to be ready for webhooks to work
go awctrl.SetupControllers(ctx, mgr, cfg.AppWrapper, certsReady, setupLog)
exitOnError(awctrl.SetupIndexers(ctx, mgr, cfg.AppWrapper), "unable to setup indexers")
go awctrl.SetupControllers(ctx, mgr, cfg.AppWrapper.Config, certsReady, setupLog)
exitOnError(awctrl.SetupIndexers(ctx, mgr, cfg.AppWrapper.Config), "unable to setup indexers")
} else if err1 != nil || err2 != nil {
exitOnError(err, "Could not determine if AppWrapper and Workload CRs present on cluster.")
exitOnError(err, "Could not determine if AppWrapper and Workload CRDs present on cluster.")
} else {
setupLog.Info("AppWrapper controller disabled", "Workload CRD present", v1,
"AppWrapper CRD present", v2, "Config flag value", *cfg.AppWrapper.Enabled)
}

exitOnError(awctrl.SetupProbeEndpoints(mgr, certsReady), "unable to setup probe endpoints")
Expand Down
9 changes: 8 additions & 1 deletion pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,14 @@ type CodeFlareOperatorConfiguration struct {
KubeRay *KubeRayConfiguration `json:"kuberay,omitempty"`

// AppWrapper contains the AppWrapper controller configuration
AppWrapper *awconfig.AppWrapperConfig `json:"appwrapper,omitempty"`
AppWrapper *AppWrapperConfiguration `json:"appwrapper,omitempty"`
}

type AppWrapperConfiguration struct {
// Enabled controls whether or not the AppWrapper Controller is enababled
Enabled *bool `json:"appWrapperControllerEnabled"`
// AppWrapper contains the AppWrapper controller configuration
Config *awconfig.AppWrapperConfig `json:"appwrapper,omitempty"`
}

type KubeRayConfiguration struct {
Expand Down