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

Add kylin OS support #9078

Merged
merged 1 commit into from
Aug 1, 2022
Merged

Add kylin OS support #9078

merged 1 commit into from
Aug 1, 2022

Conversation

ErikJiang
Copy link
Member

@ErikJiang ErikJiang commented Jul 10, 2022

Signed-off-by: bo.jiang bo.jiang@daocloud.io

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds Kylin Linux Advanced Server V10 support to kubespray.

Which issue(s) this PR fixes:

Fixes #9009

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add Kylin Linux support.

@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 Jul 10, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @ErikJiang. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 10, 2022
@k8s-ci-robot k8s-ci-robot requested review from bozzo and holmsten July 10, 2022 06:01
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 10, 2022
@ErikJiang ErikJiang closed this Jul 10, 2022
@ErikJiang ErikJiang changed the title Add kylin OS support WIP: Add kylin OS support Jul 10, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2022
@ErikJiang ErikJiang reopened this Jul 10, 2022
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 13, 2022
@ErikJiang ErikJiang changed the title WIP: Add kylin OS support Add kylin OS support Jul 13, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 13, 2022
@ErikJiang
Copy link
Member Author

ErikJiang commented Jul 14, 2022

@cristicalin @oomichi @floryut @ @bozzo @holmsten PTAL 😁

Copy link
Contributor

@oomichi oomichi left a comment

Choose a reason for hiding this comment

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

If implementing CI for Kylin Linux in this pull request, we can easily say this is ready to go.
Then just one question inline.

/cc @oomichi

Kylin Linux is currently only supported with containerd runtime.

**Note:** that Kylin Linux is not currently covered in kubespray CI and
support for it is currently considered experimental.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a blocker to implement CI for Kylin Linux?

Copy link
Member Author

Choose a reason for hiding this comment

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

😁 I actually don't know much about ci, could you tell me how to do it? @oomichi

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that is a good question.
I did the similar thing for RockyLinux 8 which is compatible with RHEL8/CentOS8 as #8905

The pull request would be helpful to know how to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@oomichi @cristicalin @floryut 🤔 Kylin Linux is a commercial operating system that does not provide public vagrant box and qcow2 image files. Is there any way to deal with this situation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Like in the case of RHEL, we don't explicitly support it but we support derivatives (CentOS, Alma, Rocky) and folks actively using RHEL fix stuff from time to time but we see bug reports of things not working on RHEL due to its closed repos.
If there is no way to test this in CI I don't think we should claim support for it since whatever is done now may later break without coverage; you can add documentation stating that it may be experimentally supported with whatever workarounds will be needed to make/keep it working but I personally think the task of maintaining support is going to be difficult and none of the current maintainers are users of this distribution so it's going to be difficult for us to jump in and fix anything.

Copy link
Contributor

@oomichi oomichi Jul 27, 2022

Choose a reason for hiding this comment

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

IICU the Kubespray's operating system check blocks running on Kylin OS.
It is fine for me to make the check allow running without CI.
That is the same as RHEL, we don't have any RHEL CI job but Kubespray allows running on the RHEL.
If users find an issue, they report and fix it.

@ErikJiang
Thanks for your work, please revert this pull request to the previous one which changed the check and wrote the document which explained the Kylin support is experimental.

Thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

@oomichi 😁 I have restored the PR to the previous state, if kylin can be accepted by kubespray, I will be very willing to apply to be one of the maintainers of kylin, and I believe that more Chinese developers will contribute to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ErikJiang Thank you for doing that.

I believe that more Chinese developers will contribute to this.

Yeah, I also guess so.

Copy link
Member

Choose a reason for hiding this comment

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

Or https://www.openkylin.top/index.php?lang=en ? 😆 joking, as @oomichi and @cristicalin it's fine to not have CI support and only experimental support as long as people try and fix issues with it (if anyone uses it)

Copy link
Member Author

@ErikJiang ErikJiang Aug 1, 2022

Choose a reason for hiding this comment

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

At present, kylin linux V10 is more mature than openkylin, so openkylin will not be considered in the short term. @floryut 😀

@k8s-ci-robot k8s-ci-robot requested a review from oomichi July 14, 2022 17:56
@ErikJiang ErikJiang changed the title Add kylin OS support WIP Add kylin OS support Jul 19, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 19, 2022
@ErikJiang ErikJiang changed the title WIP Add kylin OS support WIP: Add kylin OS support Jul 19, 2022
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 19, 2022
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 28, 2022
@ErikJiang ErikJiang changed the title WIP: Add kylin OS support Add kylin OS support Jul 28, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2022
Copy link
Contributor

@oomichi oomichi left a comment

Choose a reason for hiding this comment

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

/ok-to-test
/lgtm

Kylin Linux is currently only supported with containerd runtime.

**Note:** that Kylin Linux is not currently covered in kubespray CI and
support for it is currently considered experimental.
Copy link
Contributor

Choose a reason for hiding this comment

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

@ErikJiang Thank you for doing that.

I believe that more Chinese developers will contribute to this.

Yeah, I also guess so.

@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. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 28, 2022
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2022
Copy link
Member

@floryut floryut left a comment

Choose a reason for hiding this comment

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

🥂

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ErikJiang, floryut

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 Aug 1, 2022
Signed-off-by: bo.jiang <bo.jiang@daocloud.io>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2022
@oomichi
Copy link
Contributor

oomichi commented Aug 1, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2022
@k8s-ci-robot k8s-ci-robot merged commit f2f9f1d into kubernetes-sigs:master Aug 1, 2022
@ErikJiang ErikJiang deleted the kylin-linux-support branch August 8, 2022 02:53
@floryut floryut mentioned this pull request Sep 19, 2022
zexi pushed a commit to zexi/kubespray that referenced this pull request Nov 7, 2022
Signed-off-by: bo.jiang <bo.jiang@daocloud.io>
zexi pushed a commit to zexi/kubespray that referenced this pull request Nov 8, 2022
Signed-off-by: bo.jiang <bo.jiang@daocloud.io>
skw0823 pushed a commit to skw0823/kubespray that referenced this pull request Dec 13, 2022
Signed-off-by: bo.jiang <bo.jiang@daocloud.io>
nolimitkun pushed a commit to nolimitkun/kubespray that referenced this pull request Mar 19, 2023
Signed-off-by: bo.jiang <bo.jiang@daocloud.io>
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jul 2, 2023
Signed-off-by: bo.jiang <bo.jiang@daocloud.io>
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jul 7, 2023
Signed-off-by: bo.jiang <bo.jiang@daocloud.io>
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Kylin V10 OS
5 participants