Skip to content

Implement operator support for pool ownership#1393

Merged
dannyzaken merged 1 commit intonoobaa:masterfrom
bernerhat:add_env_var_core_provider
Jul 29, 2024
Merged

Implement operator support for pool ownership#1393
dannyzaken merged 1 commit intonoobaa:masterfrom
bernerhat:add_env_var_core_provider

Conversation

@bernerhat
Copy link
Contributor

Explain the changes

  1. part of the change required in Implement OCS and MCG operator support for pool ownership adds an indication to the core that the cluster is set to provider mode upon deployment.

Issues: Fixed #xxx / Gap #xxx

  1. This implementation supports RHSTOR-5187 - Support Provider & Consumer mode

Testing Instructions:

  • Doc added/updated
  • Tests added

@bernerhat bernerhat force-pushed the add_env_var_core_provider branch from a9de629 to fb1729b Compare July 22, 2024 08:23

//check if provider mode is enabled and signal the core
if util.GetAnnotationValue(r.NooBaa.Annotations, "MulticloudObjectGatewayProviderMode") == "true" {
util.GetEnvVariable(&r.CoreApp.Spec.Template.Spec.Containers[0].Env, "RESTRICT_RESOURCE_DELETION").Value = "true"
Copy link
Member

Choose a reason for hiding this comment

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

to modify the RESTRICT_RESOURCE_DELETION config you need to add the CONFIG_JS_ prefix in the env.

Suggested change
util.GetEnvVariable(&r.CoreApp.Spec.Template.Spec.Containers[0].Env, "RESTRICT_RESOURCE_DELETION").Value = "true"
util.GetEnvVariable(&r.CoreApp.Spec.Template.Spec.Containers[0].Env, "CONFIG_JS_RESTRICT_RESOURCE_DELETION").Value = "true"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack, changed.

pkg/util/util.go Outdated
}

func GetAnnotationValue(annotations map[string]string, name string) string {
val, err := annotations[name]
Copy link
Member

Choose a reason for hiding this comment

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

verify annotations != nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack, changed.

@bernerhat bernerhat force-pushed the add_env_var_core_provider branch from fb1729b to 1bc4a89 Compare July 22, 2024 12:50
pkg/util/util.go Outdated
Comment on lines +1429 to +1433
val, err := annotations[name]
if err {
return ""
}
return val
Copy link
Member

Choose a reason for hiding this comment

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

This code does the opposite of what you intended. Golang map returns ok as the second return value, so it is true if the value exist.
Also, if it does not exist, it should return a zero value, which in a string is "", so there is no need to really check for ok.

Suggested change
val, err := annotations[name]
if err {
return ""
}
return val
return annotations[name]

pkg/util/util.go Outdated
return nil
}

func GetAnnotationValue(annotations map[string]string, name string) string {
Copy link
Member

Choose a reason for hiding this comment

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

It is worth returning a boolean exists as a second return value, the same as the map return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for what purpose?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack, changed.

@bernerhat bernerhat force-pushed the add_env_var_core_provider branch 3 times, most recently from 650f620 to 5563877 Compare July 25, 2024 11:10
Copy link
Member

@dannyzaken dannyzaken left a comment

Choose a reason for hiding this comment

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

one small comment (it will probably fail the tests anyway) . otherwise approved

return nil
}

func GetAnnotationValue(annotations map[string]string, name string) (string, bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment for this function.

@dannyzaken
Copy link
Member

@bernerhat The unit tests failed due to diff with the expected generated files.
please run the following and commit

go run pkg/bundler/bundler.go deploy/ pkg/bundle/deploy.go

@bernerhat bernerhat force-pushed the add_env_var_core_provider branch 2 times, most recently from 37f8d60 to 0f3f7db Compare July 28, 2024 08:25
set an env var for the core when cluster is in provider mode and the annotation is set on the cr

Signed-off-by: Amit Berner <aberner@redhat.com>
@bernerhat bernerhat force-pushed the add_env_var_core_provider branch from 0f3f7db to 0b5591b Compare July 28, 2024 08:25
@bernerhat
Copy link
Contributor Author

@dannyzaken done

@dannyzaken dannyzaken merged commit d3d8033 into noobaa:master Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants