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

OCP / OKD Documentation and Helm Support #5004

Merged
merged 42 commits into from
Aug 24, 2023

Conversation

ArthurVardevanyan
Copy link
Contributor

@ArthurVardevanyan ArthurVardevanyan commented Dec 6, 2022

Picking up where: #2909 left off.

Related Issue: #1831

Been using Longhorn on my OKD HomeLab Since OKD/OCP 4.10/1.23

Since these Launches are around the corner, targeting:

  • OCP: 4.12 / 1.25 & 4.13 / 1.26
  • Longhorn 1.4.0 / 1.5.0

Main Modifications from Previous PR:

  • Removed Extra(?) Mount Points to Daemonset
  • Converted Policy Commands to Yaml Cluster Role RBAC
  • Removed ISCSI Setup as this is handled already by RHCOS/FCOS
  • General Documentation Updates

Testing:

  • 4.10 / 1.23:
    • 4.10.0-0.okd-2022-03-07-131213 to 4.10.0-0.okd-2022-07-09-073606: Tested, No Known Issues (Previously Used Versions)
  • 4.11 / 1.24:
  • 4.12 / 1.25:
    • 4.12.0-0.okd-2022-12-05-210624 to 4.12.0-0.okd-2022-12-11-164611: Tested, No Known Issues

Non-OKD/OCP Changes

Switched UI Deployment from default service-account to custom service account

binguo-casa and others added 8 commits August 20, 2021 14:46
Signed-off-by: Bin Guo <bin.guo@casa-systems.com>
Signed-off-by: Bin Guo <bin.guo@casa-systems.com>
Signed-off-by: Bin Guo <bin.guo@casa-systems.com>
Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
@innobead
Copy link
Member

innobead commented Dec 6, 2022

@ArthurVardevanyan it seems you are active on OKD deployment, so do you want to own #1831?

Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
@ArthurVardevanyan
Copy link
Contributor Author

ArthurVardevanyan commented Dec 11, 2022

4.11.0-0.okd-2022-11-19-050030 [Longhorn 1.3.2]

Their may be some noise, this a long-lasting cluster.
longhorn-support-bundle_00e45f14-1554-4313-bb2b-27dbf74ab4c7_2022-12-11T22-25-07Z.zip

Screenshot from 2022-12-11 17-05-26

4.12.0-0.okd-2022-12-11-164611 [Longhorn 1.4.0.-dev]

Their might be some noise from a rolling reboot.
New Support Bundle Feature still isn't complete:
Temporary Workaround, didn't include in the chart, since the final service account is unknown at this time.

- kind: ServiceAccount
  name: default # supportbundle-agent-support-bundle daemonset doesn't use a dedicated service account.
  namespace: {{ include "release_namespace" . }}

supportbundle_19d7497d-0deb-425a-990d-a2a6e367b09e_2022-12-11T23-17-27Z.zip
Screenshot from 2022-12-11 18-29-01

@ArthurVardevanyan ArthurVardevanyan marked this pull request as ready for review December 11, 2022 23:31
@ArthurVardevanyan
Copy link
Contributor Author

@ArthurVardevanyan it seems you are active on OKD deployment, so do you want to own #1831?

Sure, No Preference either way.
I have no plans to stop using Longhorn + OKD

This PR should be good to take a look at. It covers the basic use cases.

Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
@innobead innobead requested a review from a team December 14, 2022 21:10
Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
@innobead innobead requested review from mantissahz and removed request for weizhe0422 July 18, 2023 06:10
@innobead
Copy link
Member

@mantissahz Please help review this PR and work with @ArthurVardevanyan . Thanks.

mantissahz
mantissahz previously approved these changes Jul 24, 2023
Copy link
Contributor

@mantissahz mantissahz left a comment

Choose a reason for hiding this comment

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

In general LGTM

chart/ocp-readme.md Outdated Show resolved Hide resolved
chart/templates/clusterrole.yaml Show resolved Hide resolved
Comment on lines 307 to 308
# finalizers: true
# privileged: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we set these to true by default once openshift is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@mantissahz
Copy link
Contributor

mantissahz commented Aug 7, 2023

@ArthurVardevanyan could you please try to resolve the conversions?

I had verified the PR on OKD 4.14 environment.
By the steps in README.md (chart/ocp-readme.md):

  1. Label nodes
  2. Create a file system and the mount point for disks on nodes
  3. Create a deploying manifest file from Helm by commands
helm template longhorn --namespace longhorn-system --values values.yaml --no-hooks  > longhorn.yaml
oc create namespace longhorn-system -o yaml --dry-run=client | oc apply -f -
oc apply -f longhorn.yaml -n longhorn-system

Copy link
Member

@innobead innobead left a comment

Choose a reason for hiding this comment

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

In general, LGTM.

Please fix the remaining comments. Thanks.

chart/values.yaml Outdated Show resolved Hide resolved
deploy/longhorn.yaml Outdated Show resolved Hide resolved
name: longhorn-ui-service-account
namespace: {{ include "release_namespace" . }}
- kind: ServiceAccount
name: default # supportbundle-agent-support-bundle uses default sa
Copy link
Member

@innobead innobead Aug 21, 2023

Choose a reason for hiding this comment

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

cc @c3y1huang , maybe we should pass the service account to the support bundle manager, and it can use it for the agent pod creation as well.

After that, we can remove this binding.

chart/values.yaml Outdated Show resolved Hide resolved
@innobead
Copy link
Member

@mantissahz Please help re-verify this PR after @ArthurVardevanyan resolves the comments. Thanks.

Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
… into okd-ocp

Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
Copy link
Member

@innobead innobead left a comment

Choose a reason for hiding this comment

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

LGTM

@innobead innobead enabled auto-merge (squash) August 24, 2023 11:11
@innobead innobead disabled auto-merge August 24, 2023 11:11
@innobead innobead merged commit 67b4b38 into longhorn:master Aug 24, 2023
3 checks passed
@innobead
Copy link
Member

@ArthurVardevanyan I would like to have you help with the following contribution for OCP/OKD support to make sure it's sustained. Thanks first.

@mantissahz Please move forward with the QA process. Thanks.

@ArthurVardevanyan ArthurVardevanyan deleted the okd-ocp branch August 24, 2023 12:26
derekbit pushed a commit to derekbit/longhorn that referenced this pull request Dec 18, 2023
* Add OCP support to helm chart.

Signed-off-by: Bin Guo <bin.guo@casa-systems.com>
Signed-off-by: Arthur <arthur@arthurvardevanyan.com>
Co-authored-by: Bin Guo <bin.guo@casa-systems.com>
Co-authored-by: David Ko <dko@suse.com>
Co-authored-by: binguo-casa <70552879+binguo-casa@users.noreply.github.com>
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.

None yet

9 participants