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

feat: support blobfuse-proxy on OpenShift #1045

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

cvvz
Copy link
Member

@cvvz cvvz commented Oct 13, 2023

What type of PR is this?
/kind feature

What this PR does / why we need it:
support blob-csi-driver in openshift
Which issue(s) this PR fixes:

Fixes #791

Requirements:

Special notes for your reviewer:
in coreos, we could just copy the blobfuse2 binary to /usr/local/bin/blobfuse2

Release note:

support blob-csi-driver in openshift

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 13, 2023
@cvvz
Copy link
Member Author

cvvz commented Oct 13, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 13, 2023
pkg/blobfuse-proxy/blobfuse-proxy.service Outdated Show resolved Hide resolved
pkg/blobfuse-proxy/init.sh Outdated Show resolved Hide resolved
@andyzhangx
Copy link
Member

/assign @jsafrane

@andyzhangx andyzhangx changed the title support blob-csi-driver in openshift feat: support blobfuse-proxy on OpenShift Oct 13, 2023
Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

since there are a few different behaviors between Ubuntu and OpenShift, e.g.

  1. blobfuse-proxy path
  2. blobfuse install
  3. other os related env changes

I think it's better making separate init.sh and blobfuse-proxy.service for Ubuntu and CoreOS, that would version upgrade as little risky as possible.

@cvvz
Copy link
Member Author

cvvz commented Oct 16, 2023

I've split the installation script into two different scripts for different Linux dstros. Please take another look. @andyzhangx @jsafrane

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2023
Copy link

@jsafrane jsafrane left a comment

Choose a reason for hiding this comment

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

From what I can tell without testing, this looks good for RHCOS.

@@ -0,0 +1,68 @@
#!/bin/sh

# Copyright 2019 The Kubernetes Authors.

Choose a reason for hiding this comment

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

Nit: 2024

Comment on lines 101 to +104
- name: host-usr
mountPath: /host/usr
- name: host-usr-local
mountPath: /host/usr/local

Choose a reason for hiding this comment

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

Is this necessary if you have already /host/usr ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, for coreOS, /usr/ and /usr/local are different mount point.

Copy link
Member

Choose a reason for hiding this comment

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

I think what @jsafrane mean is that since L102 is already mounting with /usr, why not resuse this mount point

Copy link
Member Author

@cvvz cvvz Nov 7, 2023

Choose a reason for hiding this comment

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

Becase for coreOS, /usr/local is not a subpath of /usr, it's a different mount point, so we have to mount /usr/local and /usr seperately.

@rh-aelgendy
Copy link

Hi @feiskyer, It seems that the PR is pending your approval currently, please let us know if any further contribution is needed from Red Hat end in order to successfully proceed with the testing.

@andyzhangx
Copy link
Member

Hi @feiskyer, It seems that the PR is pending your approval currently, please let us know if any further contribution is needed from Red Hat end in order to successfully proceed with the testing.

@rh-aelgendy this PR has conflict now, weizhi is on vacation, we will try to merge this PR next week when weizhi is back.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2023
@andyzhangx
Copy link
Member

/retest

commit 0c185e7
Merge: bd1e028 3f47b3e
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Nov 6 05:27:11 2023 +0000

    Merge branch 'master' of https://github.com/kubernetes-sigs/blob-csi-driver into support-openshift

commit bd1e028
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Oct 18 08:40:28 2023 +0000

    fix

commit 6270feb
Merge: 697f9ab b99e1cf
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Oct 18 07:30:33 2023 +0000

    Merge branch 'master' of https://github.com/kubernetes-sigs/blob-csi-driver into support-openshift

commit 697f9ab
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Oct 16 08:07:59 2023 +0000

    arm

commit c79dc0f
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Oct 16 07:16:45 2023 +0000

    fix

commit 7096aa8
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Oct 16 07:06:17 2023 +0000

    fix

commit 617ea0e
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Oct 16 06:48:07 2023 +0000

    add mariner

commit 2f4473c
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Oct 16 06:06:48 2023 +0000

    fix

commit ee5e239
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Oct 16 06:01:50 2023 +0000

    fix

commit fd8e1f5
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Oct 16 05:47:02 2023 +0000

    fix

commit babe2f0
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Oct 16 05:44:54 2023 +0000

    fix

commit 6ce12c8
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Oct 16 04:17:51 2023 +0000

    support openshift

commit 38f50a9
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Oct 16 02:48:57 2023 +0000

    Revert "support openshift"

    This reverts commit e15df33.

commit e15df33
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Oct 13 07:27:24 2023 +0000

    support openshift
@cvvz
Copy link
Member Author

cvvz commented Nov 7, 2023

/retest

2 similar comments
@cvvz
Copy link
Member Author

cvvz commented Nov 7, 2023

/retest

@cvvz
Copy link
Member Author

cvvz commented Nov 8, 2023

/retest

Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, cvvz, jsafrane

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2023
@andyzhangx andyzhangx merged commit 4e301e7 into kubernetes-sigs:master Nov 8, 2023
21 of 22 checks passed
@andyzhangx
Copy link
Member

/cherrypick release-1.23

@k8s-infra-cherrypick-robot

@andyzhangx: #1045 failed to apply on top of branch "release-1.23":

Applying: Squashed commit of the following:
.git/rebase-apply/patch:660: trailing whitespace.
  
.git/rebase-apply/patch:667: trailing whitespace.
  # when running dpkg -i /etc/packages-microsoft-prod.deb, need to enter y to continue. 
warning: 2 lines add whitespace errors.
warning: Cannot merge binary files: charts/latest/blob-csi-driver-v0.0.0.tgz (HEAD vs. Squashed commit of the following:)
Using index info to reconstruct a base tree...
M	charts/latest/blob-csi-driver-v0.0.0.tgz
M	charts/latest/blob-csi-driver/templates/csi-blob-node.yaml
M	charts/latest/blob-csi-driver/values.yaml
M	deploy/csi-blob-node.yaml
M	pkg/blobplugin/Dockerfile
M	pkg/util/util.go
M	pkg/util/util_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/util/util_test.go
Auto-merging pkg/util/util.go
Auto-merging pkg/blobplugin/Dockerfile
Auto-merging deploy/csi-blob-node.yaml
Auto-merging charts/latest/blob-csi-driver/values.yaml
Auto-merging charts/latest/blob-csi-driver/templates/csi-blob-node.yaml
Auto-merging charts/latest/blob-csi-driver-v0.0.0.tgz
CONFLICT (content): Merge conflict in charts/latest/blob-csi-driver-v0.0.0.tgz
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Squashed commit of the following:
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.23

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@andyzhangx
Copy link
Member

@cvvz could you cherrypick to release-1.23 manually?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

blobfuse-proxy support core os (OpenShift)
6 participants