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.

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

redhat-dotnet-imagestreams: add .NET 9.0 images, remove EOL .NET 7 images. #1653

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

tmds
Copy link
Contributor

@tmds tmds commented Oct 22, 2024

No description provided.

@tmds
Copy link
Contributor Author

tmds commented Oct 22, 2024

@omajid @aslicerh @komish ptal.

Besides adding the 9.0 version and removing the 7.0 version, I've also refactored the chart so it uses templates to avoid the duplication. For future version updates we can then limit ourselves to changing the Chart.yaml and data.yaml files.

The goal is to merge this at the 9.0 GA which will then make the new chart available to existing OpenShift installations.

Copy link

@omajid omajid left a comment

Choose a reason for hiding this comment

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

Thanks, this is amazing!

Copy link
Collaborator

@komish komish left a comment

Choose a reason for hiding this comment

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

@tmds

The chart is currently failing chart testing when chart-verifier is running against it. I know we're only installing imagestreams here, but does it make sense to include a basic test that confirms the imagestream installed without issue?

@tmds
Copy link
Contributor Author

tmds commented Nov 4, 2024

does it make sense to include a basic test that confirms the imagestream installed without issue?

I'm trying to figure out what looks like.

I see many charts include a templates/tests directory, but it seems they aren't using the image streams in the test definition. Instead they refer directly to an registry.access.redhat.com image. For example: https://github.com/openshift-helm-charts/charts/blob/main/charts/redhat/redhat/redhat-python-imagestreams/0.0.2/src/templates/tests/test-import-imagestream.yaml#L12

Do these tests rely on the presence of any test causing the image streams to get imported before running that test, and in that manner verify the imagestreams are importable?

@komish
Copy link
Collaborator

komish commented Nov 5, 2024

Do these tests rely on the presence of any test causing the image streams to get imported before running that test, and in that manner verify the imagestreams are importable?

Yes. Traditional helm testing (e.g. helm test <release> will let the user install the release, then explicitly invoke its tests afterwards. Our pipeline uses chart-testing which completes both of those tasks, so it's fair to conclude that the chart will be installed and then the test will be executed. Therefore, if a test is executed, then it's reasonable to assume the helm installation phase has succeeded. Especially for these smaller charts that install things like imagestreams, which aren't technically "workloads".

I see many charts include a templates/tests directory, but it seems they aren't using the image streams in the test definition. Instead they refer directly to an registry.access.redhat.com image. For example: https://github.com/openshift-helm-charts/charts/blob/main/charts/redhat/redhat/redhat-python-imagestreams/0.0.2/src/templates/tests/test-import-imagestream.yaml#L12

To my eye, this test looks invalid. pod.spec.lookupPolicy doesn't appear to be a valid configuration. There's a good chance that imagestream chart was accepted via the "demotion" path that the pipeline offers to redhat charts, which makes helm lint the only "requirement".

I think a valid test might look something like this:

apiVersion: v1
kind: Pod
metadata:
  name: "{{ .Release.Name }}-imagestream-test"
  namespace: "{{ .Release.Namespace }}"
  annotations:
    "helm.sh/hook": test
    "alpha.image.policy.openshift.io/resolve-names": '*'
spec:
  containers:
    - name: dotnet-test
      image: dotnet:8.0-ubi8
      imagePullPolicy: IfNotPresent
      command:
        - '/bin/bash'
        - '-ec'
        - 'echo helloworld'
    - name: dotnet-runtime
      image: dotnet-runtime:8.0
      imagePullPolicy: IfNotPresent
      command:
        - '/bin/bash'
        - '-ec'
        - 'echo helloworld'
  restartPolicy: Never

Notably, the alpha.image.policy.openshift.io/resolve-names seems to be a required annotation that allows a Pod to resolve the shortname reference of the imagestream. You can probably optimize this a bit more to avoid hardcoding the image shortname references. Either way, in my use of this test, I get a successful test outcome against an arbitrary release of this chart in an arbitrary namespace.

The ability to install this chart in any namespace is critically important. I see your values.yaml includes a namespace definition, but it doesn't seem to be referenced anywhere. In our hosted pipeline, charts are installed to a fresh namespace, so if this hardcoded namespace 'openshift' were actually used, I'm sure this test would fail.

Hope this helps. Let me know how else I can assist.

@tmds
Copy link
Contributor Author

tmds commented Nov 6, 2024

The ability to install this chart in any namespace is critically important. I see your values.yaml includes a namespace definition, but it doesn't seem to be referenced anywhere. In our hosted pipeline, charts are installed to a fresh namespace, so if this hardcoded namespace 'openshift' were actually used, I'm sure this test would fail.

This was also copied from other imagestream charts in the repo. Probably this issue applies to all of them.

@tmds
Copy link
Contributor Author

tmds commented Nov 6, 2024

@komish I've added a test based on your example. I assume it is not running in CI currently? Can you trigger it to know how it behaves?

Also: what should I do about the values.yaml namespace? I went through some charts in the repo, but I haven't found the answer.

@komish
Copy link
Collaborator

komish commented Nov 6, 2024

@tmds

I've added a test based on your example. I assume it is not running in CI currently? Can you trigger it to know how it behaves?

Easiest thing to do is install chart-testing and install your chat from a local repo. Requires some setup, but will be the fastest thing.

Otherwise, just helm install and helm test. It's not purely apples-to-apples, but it's close enough.

Also: what should I do about the values.yaml namespace? I went through some charts in the repo, but I haven't found the answer.

I don't think you need this, but it's up to you and your chart's design. Ultimately, Helm provides .Release.Namespace which is most likely set by either the --namespace flag passed into helm install. Alternatively, if you don't provide one, I would presume that your kubeconfig's current-context drives what namespace it gets installed to.

All that to say - you have options here, and I'm not sure if namespace is necessary in your values.yaml. To that end, you should be okay to remove it so long as it makes sense to you.

@tmds
Copy link
Contributor Author

tmds commented Nov 7, 2024

Otherwise, just helm install and helm test. It's not purely apples-to-apples, but it's close enough.

This is what I ended up doing.

All that to say - you have options here, and I'm not sure if namespace is necessary in your values.yaml. To that end, you should be okay to remove it so long as it makes sense to you.

I've removed it, and I think it would make sense for the other charts to do the same. The parameter implies the install would be in that namespace, but as you have pointed out: it uses the current context instead.

The test is now checking all tags of the image streams and whether those include the right .NET versions for runtime/SDK.
It uncovered a mistake I had made for the runtime image repository names.

@komish thanks for all your help!!

@komish
Copy link
Collaborator

komish commented Nov 8, 2024

@tmds Some of your new tests are failing. Specifically, I see those for .NET 9* images and the "latest" image (presumably because version 9 is your latest tag) are hitting imagePullBackOff.

This is what I get when I run helm test on a fresh cluster.

waiting:
  message: Back-off pulling image "image-registry.openshift-image-registry.svc:5000/dotnet-testing/dotnet:9.0"
  reason: ImagePullBackOff
waiting:
  message: Back-off pulling image "image-registry.openshift-image-registry.svc:5000/dotnet-testing/dotnet:9.0-ubi8"
  reason: ImagePullBackOff
waiting:
  message: Back-off pulling image "image-registry.openshift-image-registry.svc:5000/dotnet-testing/dotnet:latest"
  reason: ImagePullBackOff

Here are the conditions on the ImageStreams for the v9* tags

  - conditions:
    - generation: 2
      lastTransitionTime: "2024-11-08T15:12:24Z"
      message: 'Internal error occurred: registry.access.redhat.com/ubi8/dotnet-90:9.0:
        unsupported: This repo requires terms acceptance and is only available on
        registry.redhat.io'
      reason: InternalError
      status: "False"
      type: ImportSuccess
    items: null
    tag: "9.0"
  - conditions:
    - generation: 2
      lastTransitionTime: "2024-11-08T15:12:24Z"
      message: 'Internal error occurred: registry.access.redhat.com/ubi8/dotnet-90:9.0:
        unsupported: This repo requires terms acceptance and is only available on
        registry.redhat.io'
      reason: InternalError
      status: "False"
      type: ImportSuccess
    items: null
    tag: 9.0-ubi8
  • Are the links your data.yaml correct?
  • Do these images require terms acceptance before they can be pulled?
  • How would the average user pull them when installing this chart?

@tmds
Copy link
Contributor Author

tmds commented Nov 8, 2024

.NET 9 images haven't been released yet. That will happen in the next week(s). The test should pass then, and then we can merge this.

@komish
Copy link
Collaborator

komish commented Nov 8, 2024

Whoops! Makes sense. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants