Skip to content

[patch] Don't set SLS domain by default #1822

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

Merged
merged 5 commits into from
Jul 16, 2025
Merged

[patch] Don't set SLS domain by default #1822

merged 5 commits into from
Jul 16, 2025

Conversation

rawa-resul
Copy link
Contributor

@rawa-resul rawa-resul commented Jul 4, 2025

Issue

  • Reported by customers and IBMers

Description

  • Newer versions of OCP implement stricter rules when it comes to api versioning. SLS up until 3.12.0 creates a Route using v1 as apiVersion but it needs to be route.openshift.io/v1 otherwise fails. This is fixed in 3.12.1.
  • By default, the sls role would set the sls_domain which forces SLS to create a Route. This logic is wrong. By removing this we also create a workaround for SLS up to 3.12.0 to bypass this issue.

Test Results

  • Ran the sls role to install SLS without providing sls_domain --> Installed SLS using service url.
  • Ran the sls role to install SLS with providing sls_domain --> Installed SLS and created Route.
  • Ran the sls role to install SLS without providing sls_domain then ran role again with sls_domain --> Installed SLS and created Route.
  • Ran the sls role to install SLS with providing sls_domain then ran role again without sls_domain --> Installed SLS and created Route; second run did not remove Route.
  • Attached this branch of api-licensing in my branch of the CLI and ran a mas install successfully.

⚠️ Notes for Reviewers

  • Ensure you have understood the PR guidelines in the Playbook before proceeding with a review.
  • Ensure all sections in the PR template are appropriately completed.

@rawa-resul rawa-resul self-assigned this Jul 4, 2025
@rawa-resul rawa-resul marked this pull request as ready for review July 15, 2025 20:40
Copy link
Contributor

@durera durera left a comment

Choose a reason for hiding this comment

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

Did we set up existing MAS instances to use the route that we are now effectively deprecating (in most scenario)?

I assume the result of this change is that the slscfg that will be used in most installs will now be one that is using the kubernetes service.

What is the expected behaviour in this scenario:

  • Install first MAS instance + SLS using older version of the CLI
  • Install second MAS instance using new version of the CLI

When the second install runs it will refresh the SLS instance ... and presumably this will result in the removal of the route. So did we set up the first MAS instance using that route?

@rawa-resul
Copy link
Contributor Author

Did we set up existing MAS instances to use the route that we are now effectively deprecating (in most scenario)?

I assume the result of this change is that the slscfg that will be used in most installs will now be one that is using the kubernetes service.

What is the expected behaviour in this scenario:

  • Install first MAS instance + SLS using older version of the CLI
  • Install second MAS instance using new version of the CLI

When the second install runs it will refresh the SLS instance ... and presumably this will result in the removal of the route. So did we set up the first MAS instance using that route?

When you install using an older version of CLI, SLS will have a Route. If you then install over it using a newer CLI with this change then nothing will happen to the Route. The domain field will still be present in the LicenseService CR. This is the behaviour I was aiming for. I did not want to break anyone's environment if they already have a Route because they could be using it off-cluster too.

When patching the LicenseService CR with a spec that doesn't have the domain field would not remove it. By default, Kubernetes only updates fields that are in both the local template and CR, and adds the remaining new fields.

Moreover, SLS does not currently have the logic of removing a Route when the domain field is removed from the CR. This was discovered during this work and I will add that into SLS.

@durera durera added this pull request to the merge queue Jul 16, 2025
Merged via the queue into master with commit 4831d50 Jul 16, 2025
2 checks passed
@durera durera deleted the sls-domain-fix branch July 16, 2025 22:25
karol-czarnecki added a commit that referenced this pull request Jul 21, 2025
commit 593108c
Author: karol-czarnecki <[email protected]>
Date:   Mon Jul 21 07:41:02 2025 +0100

    [patch] Fix airgap support for kmodels in AI Service (#1832)

commit 9bdf64f
Author: David Parker <[email protected]>
Date:   Thu Jul 17 10:27:07 2025 +0100

    [patch] Fix min release for odf-dependencies (#1837)

commit cafe13f
Author: faangbait <[email protected]>
Date:   Thu Jul 17 03:38:22 2025 -0500

    [patch] Remove spec.name from Grafana v5 dashboards (#1818)

commit 16854a2
Author: Terence Quinn <[email protected]>
Date:   Thu Jul 17 03:32:39 2025 -0500

    [minor] Add support for image mirroring to AWS ACR (#1791)

commit 3c55c38
Author: faangbait <[email protected]>
Date:   Thu Jul 17 03:30:21 2025 -0500

    [patch] Shorten and standardize s3 bucket access point name (#1739)

commit 8a9648c
Author: caemar <[email protected]>
Date:   Thu Jul 17 10:28:59 2025 +0200

    [patch] Add options to custom-archive-credentials secret (#1585)

commit 44bb896
Author: David Parker <[email protected]>
Date:   Thu Jul 17 09:24:52 2025 +0100

    [patch] Set Fyre master nodes to 4cpu 8gb mem (#1836)

commit 2573a5c
Author: joaopauloksn <[email protected]>
Date:   Wed Jul 16 19:46:45 2025 -0300

    [major] Avoid usage of an additional variable to install Manage foundation (#1807)

    Co-authored-by: Joao Paulo Karol Nunes <[email protected]>

commit 4831d50
Author: Rawa <[email protected]>
Date:   Wed Jul 16 23:22:40 2025 +0100

    [patch] Don't set SLS domain by default (#1822)

    Co-authored-by: Rawa Resul <[email protected]>
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.

2 participants