Skip to content
This repository was archived by the owner on Feb 6, 2025. It is now read-only.

Enable toggling of the CNI plugin used in the cluster #1162

Merged
merged 2 commits into from
Jun 23, 2020

Conversation

rktidwell
Copy link
Contributor

@rktidwell rktidwell commented Jun 12, 2020

Why is this PR needed?

Introduce special handling of CNI addons

This change introduces special handling of addons that introduce CNI plugins. The change introduces the following new functionality:

- Introduce '--cni-plugin' flag for use with 'skuba cluster init'
- Addons must now declare whether they provide a CNI plugin or not
- The user-supplied CNI plugin request is checked against addons that provide
  CNI plugins
- If '--cni-plugin' is not specified, cilium selected as the default CNI
  addon. This maintains backward-compatible behavior for 'skuba cluster init'
- Rather than write config files for all CNI plugin addons,
  configuration files are only written for the selected CNI addon

This enables future support of CNI plugins beyond Cilium.

What does this PR do?

This introduces a new optional flag for skuba cluster init that allows for specifying the CNI plugin to configure in the cluster. This change does not add suport for any new CNI's, it simply creates the framework by which new CNI plugins can be integrated. Support for new CNI plugins will be handled with future PR's.

--cni-plugin is an optional flag, and at this time only allows cilium to be passed as an option. These changes are backward-compatible: invoking skuba cluster init without --cni-plugin will emit manifests for Cilium as has been previously done. As more CNI plugins are added, they can be specified using skuba cluster init --cni-plugin <CNI plugin name>.

Anything else a reviewer needs to know?

This PR adds automated test cases. All tests we have had in place are still valid.

Info for QA

N/A

Related info

Docs

If docs need to be updated, please add a link to a PR to https://github.com/SUSE/doc-caasp.
At the time of creating the issue, this PR can be work in progress (set its title to [WIP]),
but the documentation needs to be finalized before the PR can be merged.

Merge restrictions

(Please do not edit this)

We are in v4-maintenance phase, so we will restrict what can be merged to prevent unexpected surprises:

What can be merged (merge criteria):
    2 approvals:
        1 developer: code is fine
        1 QA: QA is fine
    there is a PR for updating documentation (or a statement that this is not needed)

Copy link
Contributor

@manuelbuil manuelbuil left a comment

Choose a reason for hiding this comment

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

I can't think of a better approach. I guess the only bad consequence is that we need to load all cni plugins but that should be fine as I guess we will not have a lot of them.

I am wondering about upgrades: we run a command that upgrades all the addons and I fear that, if we have CNI_A and CNI_B, then the system will apply both addons in an upgrade and thus deploy both CNIs. If I recall correctly, this is the function that gets called by the upgrade: https://github.com/SUSE/skuba/blob/master/internal/pkg/skuba/addons/addons.go#L150 and it seems it fetches all addons, so we might get in trouble.

As I don't think we support deletion of addons, as long as we have deployment and upgrade working, we should be fine

@rktidwell
Copy link
Contributor Author

I am wondering about upgrades: we run a command that upgrades all the addons and I fear that, if we have CNI_A and CNI_B, then the system will apply both addons in an upgrade and thus deploy both CNIs.

Yes, upgrades will need to be handled with some special-casing as well. This PR isn't meant to address the upgrade case, although it might not be too painful to include those changes here. I should update the commit messages and such to reflect that.

@rktidwell
Copy link
Contributor Author

I believe the latest changes I pushed will handle upgrades and node actions cleanly. I'll continue testing to validate.

@rktidwell rktidwell force-pushed the master branch 2 times, most recently from 5fb94ca to 1c693a5 Compare June 17, 2020 18:44
@rktidwell rktidwell requested a review from jenting June 17, 2020 18:44
Copy link

@soumynathan soumynathan left a comment

Choose a reason for hiding this comment

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

Comments inline.

Copy link

@jenting jenting left a comment

Choose a reason for hiding this comment

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

Please fix linter error, run make lint && git status

@rktidwell
Copy link
Contributor Author

Please fix linter error, run make lint && git status

make lint has been succeeding locally. CI is complaining about not having go 1.13 in go.mod, which is unrelated to this change. Do I need to rebase? Are we adding this to go.mod now?

This change breaks out Init() into smaller functions to thematically
group them together. This enhances readability and sets the stage for
supporting enhancements to addon handling.

Signed-off-by: Ryan Tidwell <rtidwell@suse.com>
@rktidwell
Copy link
Contributor Author

Please fix linter error, run make lint && git status

make lint has been succeeding locally. CI is complaining about not having go 1.13 in go.mod, which is unrelated to this change. Do I need to rebase? Are we adding this to go.mod now?

I went ahead and rebased. Looks like some changes were pushed between when I created my fork and creating this PR. Things are looking better now.

@rktidwell rktidwell requested a review from jenting June 18, 2020 02:49
flavio
flavio previously approved these changes Jun 18, 2020
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

Overall looks good

This change introduces special handling of addons that introduce CNI
plugins. The change introduces the following new functionality:

- Introduce '--cni-plugin' flag for use with 'skuba cluster init'
- Addons now declare their AddOnType, allowing for custom logic based on
  AddOnType to be invoked when writing out initial cluster manifests
- The user-supplied CNI plugin request is checked against addons that provide
  CNI plugins
- If '--cni-plugin' is not specified, cilium selected as the default CNI
  addon. This maintains backward-compatible behavior for 'skuba cluster init'
- Rather than write config files for all CNI plugin addons,
  configuration files are only written for the selected CNI addon
- CNI addons that are not applicable will not be applied during
  upgrades or node actions such as bootstrap or join

Signed-off-by: Ryan Tidwell <rtidwell@suse.com>
Copy link

@jenting jenting left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants