RHOAI-4148: Re-adjust the statefulset, route, service based on generateName

[harshad16]

https://issues.redhat.com/browse/RHOAIENG-4148

Description

Re-adjust the statefulset, route, service based on generateName

The setup on Opendatahub allows spin-up of workbenches, via

• Data Science Project
• Jupyter Tile (application>enabled)

Way of naming in the above methods:

• Data Science Project: users choose to name it
• Jupyter Tile: jupyter-nb-<encoded-username>

Length issue on Jupyter Tile:

• Depending on the username, the dashboard encode special characters as not special chars are accepted in Kubernetes.
  • This can cause the length of the username to change with few added chars.
• Dashboard adds jupyter-nb- that is 11 len, here.

Explanation:

• The route URL is created by sub-domain(<route-name>-<namespace>) : <route-name>-<namespace>.<domain>
• The len of sub-domain needs to be 63 chars. References:
  • https://docs.openshift.com/container-platform/3.11/architecture/networking/routes.html#:~:text=Each%20route%20consists%20of%20a,and%20an%20optional%20security%20configuration.
  • https://access.redhat.com/solutions/3638581
• As jupyter-tiles are created in namespace (rhods-notebooks) this add: 16 chars to sub-domain.
• Following get the limit of len of username: 63-16 = 47 - (len of prefix jupyter-nb i.e 11) = 36.

In ODH username above 36 chars were failing , fixing the Route name to generated using Kubernets generateName method, we can fix the route sub-domain issue.

Upon further exploring found that labels have length constraint of 63 chars.

• https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names
• https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set

The controllerRevision, adds a hash that is 8-10Chars, which adds 11 chars to a label of statefulset.
Given a workbench in jupyter-tile, that allows:
username: 63- (len of prefix jupyter-nb i.e 11) - (len of hash i.e 11) = 41

In ODH username between 36-41 chars were failing in statefulset creation stage , fixing the Statefulset name to generated using Kubernets generateName method, we can fix the controller-revision-hash label.

With this, username can be up to 52 chars.
as there are other labels, which would fail the 63 chars limit (app, notebook-name labels)

End Result:
Old username limit: max 36
New username limit: max 52

Related-to:
#179
https://issues.redhat.com/browse/RHOAIENG-4148

How Has This Been Tested?

TBD

Merge criteria:

• The commits are squashed in a cohesive manner and have meaningful messages.
• Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
• The developer has manually tested the changes and verified that the changes work

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 51.72414% with 70 lines in your changes missing coverage. Please review.

Project coverage is 55.28%. Comparing base (690694f) to head (c156792).

Files with missing lines	Patch %	Lines
...-notebook-controller/controllers/notebook_route.go	42.55%	49 Missing and 5 partials ⚠️
...book-controller/controllers/notebook_controller.go	53.33%	11 Missing and 3 partials ⚠️
...-notebook-controller/controllers/notebook_oauth.go	90.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #539      +/-   ##
==========================================
- Coverage   56.47%   55.28%   -1.20%     
==========================================
  Files          10       10              
  Lines        2686     2780      +94     
==========================================
+ Hits         1517     1537      +20     
- Misses       1054     1120      +66     
- Partials      115      123       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jiridanek]

This is good work, but I worry it's not really all that much helpful to fix https://issues.redhat.com/browse/RHOAIENG-4148

For odh/kubeflow, the thing to help with the customer issue, would be to reject Notebook CR creation immediately, instead of getting stuck creating route, or whatever, later.

The largest weight lays on Dashboard which simply cannot put the username into the Notebook CR name, because the usernames can be long.

It is a good change, but it will not resolve RHOAIENG-4148 alone.

Also, it is getting us towards

• fix: resolve service name conflict for notebooks and tensorboards kubeflow/kubeflow#7468 (comment)

which is probably a good direction to move towards.

[jiridanek]

Ftr, there's a new feature in rhods operator to let admin change the Jupyterlab namespace, https://issues.redhat.com/browse/RHOAIENG-22096

Default still is

 ${DEFAULT_WORKBENCHES_NAMESPACE}=    rhods-notebooks

[harshad16]

Ftr, there's a new feature in rhods operator to let admin change the Jupyterlab namespace, https://issues.redhat.com/browse/RHOAIENG-22096

Default still is

 ${DEFAULT_WORKBENCHES_NAMESPACE}=    rhods-notebooks

Great point, noticing that, I was trying to change the logic for making it more generic 👍

[jiridanek]

/lgtm

I've been thinking about this and while I still believe what I said/thought before (the change does not really fix customer issue, change will move our logic further from upstream kubeflow notebooks, the branching logic for the route takes time to understand), I don't see anything wrong with this.

In the interest of making further progress, I think we should merge this. It will fix some scenarios, upstream kubeflow is not really making any changes in the 1.x notebook controller codebase, and the logic is not too bad and worst case chatgpt can help explain if I ever stumble with it.

Ship it!

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jiridanek. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jstourac]

Do we want to do something with the failing CI code static analysis check?

prefix=components/odh-notebook-controller --timeout=5m] in [/home/runner/work/kubeflow/kubeflow/components/odh-notebook-controller] ...
  Error: components/odh-notebook-controller/controllers/notebook_route.go:106:21: ST1023: should omit type bool from declaration; it will be inferred from the right-hand side (staticcheck)
  	var isGenerateName bool = false
  	                   ^
  Error: components/odh-notebook-controller/controllers/notebook_route.go:236:5: QF1008: could remove embedded field "ObjectMeta" from selector (staticcheck)
  	sa.ObjectMeta.Annotations["serviceaccounts.openshift.io/oauth-redirectreference.second"] = "" +
  	   ^
  2 issues:
  * staticcheck: 2

AFAIK it's not failing in main?

[jstourac]

How will this look like in the real world? isn't it too generic? I mean - would it make sense to have at least some portion of the actual original instance.Name here so it's at least somehow traceable to the original user via oc get ... command?

[harshad16]

How will this look like in the real world? isn't it too generic?

As this change would only happen if isGenerateName is set, which depends on notebook name to be larger than 52 char. In ODH scenario, it is only possible via jupyter tile , as DS project has slices notebook name above 30 char.
In jupyter tile, user dont get to set the notebook name on the UI, so on console it would show up as nb-
user can query them via cli using:
oc get notebook --all-namespaces -l opendatahub.io/user={username}
or
oc get statefulset --all-namespaces -l statefulset={notebook.name}
`

we could have some part of instance.name, i wasn't sure, what slice should it be prefix or suffix end of instance.name.

[jstourac]

I understand. My intention here was to make it easier to distinguish also on the pods level - when one lists the pods from rhods-notebooks namespace (Jupyter tile), then with this change it will not be possible to match relevant pods based on their name to the users anymore at all.

I don't insist. The solution using the label may be good enough. I was just trying to be sure that introducing this change we're not making this a bit more complex for eventual customer automation they could have.

But yeah, I don't really insist.

[harshad16]

Absolutely, i do feel inline with you.
With this case being exclusive to jupyter tiles, keeping the name sequence could be a way. Perhaps we can do this in next iterations.

[daniellutz]

I understand and agree with @jiridanek's comments.

Since some of these are mostly due to OCP's own limitations and requirements, this PR will be a great fix to these issues already. Tests are present, only some linter checks appearing on CI, but everything looks good!

/lgtm

[jstourac]

/lgtm

[harshad16]

/retest-required

[harshad16]

Thank you for all the reviews
getting this in.