-
Notifications
You must be signed in to change notification settings - Fork 78
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
support universal base image #95
Conversation
operator/pkg/apis/cassandra/v1beta1/cassandradatacenter_types.go
Outdated
Show resolved
Hide resolved
…d/publish ubi images from gh workflow
if baseImageOs := os.Getenv(api.EnvBaseImageOs); baseImageOs != "" { | ||
loggerContainer.Image = baseImageOs | ||
} else { | ||
loggerContainer.Image = "busybox" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not for this PR, but we should offer a way to shut off this tailing container
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard coding specific coordinates feels like a missed mark. It would be nice to see this feature focus on specifying default repositories for each component where the UBI deployments leverage that feature instead of it being a feature that focuses on UBI.
I see environments where private registries must be used prefer this apporach as the users will not have to specify the configBuilderImage
and serverImage
.
operator/pkg/apis/cassandra/v1beta1/cassandradatacenter_types.go
Outdated
Show resolved
Hide resolved
operator/pkg/apis/cassandra/v1beta1/cassandradatacenter_types.go
Outdated
Show resolved
Hide resolved
if baseImageOs := os.Getenv(api.EnvBaseImageOs); baseImageOs != "" { | ||
loggerContainer.Image = baseImageOs | ||
} else { | ||
loggerContainer.Image = "busybox" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
ubi_cassandra_3_11_6 = "registry.connect.redhat.com/datastax/cassandra:3.11.6" | ||
ubi_cassandra_4_0_0 = "TODO" | ||
ubi_dse_6_8_0 = "registry.connect.redhat.com/datastax/dse-server:6.8.0" | ||
ubi_defaultConfigBuilderImage = "registry.connect.redhat.com/datastax/cass-config-builder:1.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're gonna stick with dockerhub, no need for registry.connect.redhat.com
in this PR because it'll be hard to test - this PR is focusing on UBI bytes, not image hosting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the images on DockerHub are suffixed with -ubi7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replacing my request for changes with the following based on a call with @jimdickinson and @sandoichi .
In the UBI environment there are some requirements for the Dockerfile. See https://github.com/DSPN/red-hat-containers/tree/master/cass-operator for the current implementation. It would be nice to remove the DSPN/red-hat-containers repo if the appropriate elements are included here.
@@ -1,56 +0,0 @@ | |||
name: Cass Operator Stable Build & Deploy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌 less duplication!
operator/cmd/manager/main.go
Outdated
} | ||
|
||
baseOs := strings.TrimSpace(string(rawVal)) | ||
os.Setenv("BASE_IMAGE_OS", baseOs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to use the EnvBaseImageOs
constant here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, good catch!
return "", false | ||
} | ||
|
||
func getImageForUniversalBaseOs(sv string) (string, bool) { | ||
switch sv { | ||
case "dse-6.8.0": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we turn this into a map somewhere at the top of the file, so we don't have to dig all the way down into these functions to update it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have plans to update some of this for 6.8.1 support in a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to request changes on the C* 4.0.0 tag since it doesn't exist yet, but that's the only issue I see.
cassandra_4_0_0 = "datastax/cassandra-mgmtapi-4_0_0:v0.1.5" | ||
dse_6_8_0 = "datastax/dse-server:6.8.0" | ||
ubi_cassandra_3_11_6 = "datastax/cassandra:3.11.6-ubi7" | ||
ubi_cassandra_4_0_0 = "datastax/cassandra:4.0-ubi7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking the tag will be datastax/cassandra:4.0.0-dev-ubi7
since there are no official tags for 4.0.0 yet. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably fine to leave as is for now to get this merged and then update it once we have the actual coordinate in place
The bulk of the changes boils down to:
Specify
MO_BASE_OS
when running the mage operator build targets or the mage k8s cluster setup targets and it will build an operator based off of the image you specify inMO_BASE_OS
, tag it with a-ubi
suffix, and then properly load it into your local cluster + override the helm chart value to use that image in the deployment.example to build the operator and get
-ubi
tagged images:example to build the operator, setup a k3d cluster, and load the image in + stand up the deployment:
The operator built for the ubi image will then always choose ubi based dse/config builder/logging container images when it stands up a datacenter.
We will now also always publish both regular and
-ubi
versions of the operator from the github workflow.