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

Support for MetalLB v0.13.9 with CRD #9120

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

Jeroen0494
Copy link
Contributor

@Jeroen0494 Jeroen0494 commented Jul 25, 2022

Hi,

My first PR for this project.

What type of PR is this?
/kind feature

What this PR does / why we need it:
MetalLB has switched from using a ConfigMap to using it's own CRD notation. This PR is a first attempt at adding support for this new notation.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
Not ready yet, but comments are welcome.

Sample output:

---

# Create all pools
apiVersion: v1
items:
- apiVersion: metallb.io/v1beta1
  kind: IPAddressPool
  metadata:
    creationTimestamp: "2022-09-06T14:28:02Z"
    generation: 1
    name: pool1
    namespace: metallb-system
    resourceVersion: "7777578"
    uid: 11233fb8-78c3-4604-a7a8-e7aec0b71d53
  spec:
    addresses:
    - 172.18.113.180/30
    autoAssign: false
    avoidBuggyIPs: true
- apiVersion: metallb.io/v1beta1
  kind: IPAddressPool
  metadata:
    creationTimestamp: "2022-09-06T14:28:03Z"
    generation: 1
    name: pool2
    namespace: metallb-system
    resourceVersion: "7777581"
    uid: bae56cfa-a5b6-4cec-bf5c-8016476ccc1d
  spec:
    addresses:
    - 172.18.113.185/30
    autoAssign: false
    avoidBuggyIPs: true
- apiVersion: metallb.io/v1beta1
  kind: IPAddressPool
  metadata:
    creationTimestamp: "2022-08-04T10:11:41Z"
    generation: 3
    name: primary
    namespace: metallb-system
    resourceVersion: "7777575"
    uid: 6825f7c0-ad82-4a37-b602-84657118013e
  spec:
    addresses:
    - 172.18.113.167-172.18.113.169
    autoAssign: true
    avoidBuggyIPs: true
kind: List



---
# Create layer2 configuration

apiVersion: v1
items:
- apiVersion: metallb.io/v1beta1
  kind: L2Advertisement
  metadata:
    creationTimestamp: "2022-08-04T10:11:43Z"
    generation: 1
    name: primary
    namespace: metallb-system
    resourceVersion: "3646"
    uid: 2f5fcb4d-6efc-48e2-9659-781bb98308c8
  spec:
    ipAddressPools:
    - primary
kind: List



---
# Create layer3 configuration

apiVersion: v1
items:
- apiVersion: metallb.io/v1beta1
  kind: Community
  metadata:
    creationTimestamp: "2022-08-04T10:11:44Z"
    generation: 1
    name: vpn-only
    namespace: metallb-system
    resourceVersion: "3647"
    uid: 4642ec54-d4c2-4818-bccb-3faf62795264
  spec:
    communities:
    - name: vpn-only
      value: "1234:1"
- apiVersion: metallb.io/v1beta1
  kind: Community
  metadata:
    creationTimestamp: "2022-08-04T10:11:44Z"
    generation: 1
    name: well-known
    namespace: metallb-system
    resourceVersion: "3648"
    uid: 2a0d5657-a40c-45c6-8a8e-e0f42c3c80d8
  spec:
    communities:
    - name: no-export
      value: 65535:65281
    - name: no-advertise
      value: 65535:65282
    - name: local-as
      value: 65535:65283
    - name: nopeer
      value: 65535:65284
kind: List

---

apiVersion: v1
items:
- apiVersion: metallb.io/v1beta1
  kind: BGPAdvertisement
  metadata:
    creationTimestamp: "2022-09-06T14:28:05Z"
    generation: 1
    name: peer1-external
    namespace: metallb-system
    resourceVersion: "7777602"
    uid: d8ff053f-edbc-462b-8e3c-c17103638f15
  spec:
    aggregationLength: 32
    aggregationLengthV6: 128
    ipAddressPools:
    - pool1
- apiVersion: metallb.io/v1beta1
  kind: BGPAdvertisement
  metadata:
    creationTimestamp: "2022-09-06T14:41:09Z"
    generation: 1
    name: peer2-external
    namespace: metallb-system
    resourceVersion: "7780518"
    uid: 786d55a4-bf06-4532-9861-e9a1452144d9
  spec:
    aggregationLength: 30
    aggregationLengthV6: 128
    communities:
    - vpn-only
    ipAddressPools:
    - pool2
- apiVersion: metallb.io/v1beta1
  kind: BGPAdvertisement
  metadata:
    creationTimestamp: "2022-09-06T14:39:41Z"
    generation: 1
    name: peer2-local
    namespace: metallb-system
    resourceVersion: "7780238"
    uid: fe74307b-f8b0-4e10-83c8-1d643f7992b8
  spec:
    aggregationLength: 32
    aggregationLengthV6: 128
    communities:
    - no-advertise
    ipAddressPools:
    - pool2
kind: List

---

apiVersion: v1
items:
- apiVersion: metallb.io/v1beta2
  kind: BGPPeer
  metadata:
    creationTimestamp: "2022-09-06T14:39:41Z"
    generation: 1
    name: peer1
    namespace: metallb-system
    resourceVersion: "7780237"
    uid: 342dfb7b-e1d4-40e8-92ad-54244cf8a955
  spec:
    holdTime: 120s
    myASN: 4200000000
    peerASN: 64512
    peerAddress: 172.18.113.162
    peerPort: 179
- apiVersion: metallb.io/v1beta2
  kind: BGPPeer
  metadata:
    creationTimestamp: "2022-09-06T14:42:01Z"
    generation: 1
    name: peer2
    namespace: metallb-system
    resourceVersion: "7780685"
    uid: ffb80828-74e2-4d7f-a594-c0062c253ccc
  spec:
    ebgpMultiHop: true
    holdTime: 60s
    myASN: 4200000000
    password: changeme
    peerASN: 64513
    peerAddress: 172.18.113.161
    peerPort: 179
    routerID: 1.2.3.5
kind: List

TODO:

  • Testing
  • Update defaults
  • Write documentation
  • Write release nodes

/sig area/apps
/sig area/apps/metallb

Does this PR introduce a user-facing change?:

Support for MetalLB v0.13.9 with CRD (⚠️ This release includes user facing changes for which there is action required.
The way the inventory is setup for MetalLB deployment has changed significantly. Most prominently, we have switched from underscores to a dictionary for defining resources. Please follow the [documentation](https://github.com/kubernetes-sigs/kubespray/blob/master/docs/metallb.md) for restructuring your MetalLB inventory variables.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 25, 2022
@k8s-ci-robot
Copy link
Contributor

@Jeroen0494: The label(s) sig/area/apps, sig/area/apps/metallb cannot be applied, because the repository doesn't have them.

In response to this:

Hi,

My first PR for this project.

What type of PR is this?
/kind feature

What this PR does / why we need it:
MetalLB has switched from using a ConfigMap to using it's own CRD notation. This PR is a first attempt at adding support for this new notation.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
Not ready yet, but comments are welcome.

Does this PR introduce a user-facing change?:
Yes, this will require a new notation for inventory variables.


Sample output:

---

# Create all pools


---
apiVersion: metallb.io/v1beta1
kind: IPAddressPool
metadata:
namespace: "metallb-system"
name: "primary"
spec:

addresses:
- "192.0.1.0-192.0.1.254"

auto-assign: "True"



---
apiVersion: metallb.io/v1beta1
kind: IPAddressPool
metadata:
namespace: "metallb-system"
name: "pool2"
spec:

addresses:
- "192.0.2.2-192.0.2.2"

auto-assign: "False"



---
apiVersion: metallb.io/v1beta1
kind: IPAddressPool
metadata:
namespace: "metallb-system"
name: "pool1"
spec:

addresses:
- "192.0.2.1-192.0.2.1"

auto-assign: "False"





# Create layer2 configuration



# L2 Configuration
apiVersion: metallb.io/v1beta1
kind: L2Advertisement
metadata:
name: "primary"
namespace: "metallb-system"
spec:
ipAddressPools:
- "primary"






# Create layer3 configuration



---
apiVersion: metallb.io/v1beta1
kind: Community
metadata:
name: "NO_ADVERTISE"
namespace: "metallb-system"
spec:
community:
- name: "NO_ADVERTISE"
value: "65535:65282"

---
apiVersion: metallb.io/v1beta1
kind: Community
metadata:
name: "vpn-only"
namespace: "metallb-system"
spec:
community:
- name: "vpn-only"
value: "1234:1"




---
apiVersion: metallb.io/v1beta1
kind: BGPAdvertisement
metadata:
name: "peer1-local"
spec:
ipAddressPools:

aggregationLength: 32
localpref: "100"
communities:

- "vpn-only"


---
apiVersion: metallb.io/v1beta1
kind: BGPAdvertisement
metadata:
name: "peer1-external"
spec:
ipAddressPools:

aggregationLength: "24"

---
apiVersion: metallb.io/v1beta2
kind: BGPPeer
metadata:
name: "peer1"
namespace: "metallb-system"
spec:
myASN: "4200000000"
peerASN: "64512"
peerAddress: "192.0.2.1"


sourceAddress: "172.18.113.161"



peerPort: "179"



password: "changeme"



routerID: "1.2.3.4"



holdTime: "120s"



keepaliveTime: "120s"






---
apiVersion: metallb.io/v1beta1
kind: BGPAdvertisement
metadata:
name: "peer2-local"
spec:
ipAddressPools:

aggregationLength: 32
localpref: "100"
communities:

- "NO_ADVERTISE"


---
apiVersion: metallb.io/v1beta1
kind: BGPAdvertisement
metadata:
name: "peer2-external"
spec:
ipAddressPools:

aggregationLength: "24"

---
apiVersion: metallb.io/v1beta2
kind: BGPPeer
metadata:
name: "peer2"
namespace: "metallb-system"
spec:
myASN: "4200000000"
peerASN: "64513"
peerAddress: "192.0.2.2"


sourceAddress: "172.18.113.162"



peerPort: "179"



password: "changeme"



routerID: "1.2.3.5"



holdTime: "120s"



keepaliveTime: "120s"

TODO:

  • Cosmetic fixes (whitespace
  • Indentation
  • Testing
  • Update defaults
  • Write documentation
  • Write release nodes

/sig area/apps
/sig area/apps/metallb

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.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 25, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Jeroen0494 / name: Jeroen Rijken (c081f7c)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 25, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @Jeroen0494!

It looks like this is your first PR to kubernetes-sigs/kubespray 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubespray has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

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

Hi @Jeroen0494. 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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 25, 2022
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2022
@Jeroen0494
Copy link
Contributor Author

Update: I'm waiting for the fix for metallb/metallb#1538 before this can be updated and merged.

@Jeroen0494
Copy link
Contributor Author

Seems no fix is needed on the MetalLB side, just better documentation and a more explicit AdminssionController error.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2022
@Jeroen0494 Jeroen0494 force-pushed the feat/metallbv0.13.4 branch 2 times, most recently from 1569cf2 to d9ea0bc Compare October 21, 2022 14:30
@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 28, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 27, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 26, 2023
@Jeroen0494
Copy link
Contributor Author

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Mar 20, 2023
@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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 4, 2023
@mzaian
Copy link
Contributor

mzaian commented Apr 11, 2023

Hi @Jeroen0494 Thank you and the PR needs rebase also would be nice if you squash your commits.

Signed-off-by: Jeroen Rijken <jeroen.rijken@xs4all.nl>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2023
@Jeroen0494
Copy link
Contributor Author

Hi @Jeroen0494 Thank you and the PR needs rebase also would be nice if you squash your commits.

Hi @mzaian, I rebased and squashed my commits.

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.

@Jeroen0494 Thank you 👍

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Apr 14, 2023
@mzaian
Copy link
Contributor

mzaian commented Apr 14, 2023

Thanks @Jeroen0494
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2023
@k8s-ci-robot k8s-ci-robot merged commit 709ae1d into kubernetes-sigs:master Apr 14, 2023
Copy link

@hufhend hufhend left a comment

Choose a reason for hiding this comment

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

I downloaded the current version and it started throwing me an error, with the previous version it runs.

ERROR! couldn't resolve module/action 'k8s'. This often indicates a misspelling, missing collection, or incorrect module path.

The error appears to be in '/home/hufhendr/git/kubespray/roles/kubernetes-apps/metallb/tasks/main.yml': line 53, column 3, but may
be elsewhere in the file depending on the exact syntax problem.

The offending line appears to be:


- name: Kubernetes Apps | Create MetalLB resources and replace existing
  ^ here

@Jeroen0494
Copy link
Contributor Author

@hufhend Thanks for reporting this. Looks like I forgot to change using the k8s module to kubectl like Kubespray does for everything. I'll fix this tomorrow.

@floryut @mzaian We haven't discussed writing release notes yet for the changes required in peoples inventory files. Shall I write a changelog and if so, where can I post it?

@Jeroen0494
Copy link
Contributor Author

The kube module used by Kubespray doesn't seem to support wait conditions. Is there a particular reason not to use the kubernetes.core.k8s module provided by Ansible?
I want to do the following:

    kind: Deployment
    namespace: metallb-system
    name: controller
    wait: True
    wait_sleep: 10
    wait_timeout: 360
    wait_condition:
      status: "True"
      type: Available

I've done my best to work with the tools provided, and included a wait for the resource creation of MetalLB itself.

@floryut
Copy link
Member

floryut commented Apr 18, 2023

@hufhend Thanks for reporting this. Looks like I forgot to change using the k8s module to kubectl like Kubespray does for everything. I'll fix this tomorrow.

@floryut @mzaian We haven't discussed writing release notes yet for the changes required in peoples inventory files. Shall I write a changelog and if so, where can I post it?

Release note are automatically generated using the PR description (release-note part)

@Jeroen0494
Copy link
Contributor Author

@hufhend Thanks for reporting this. Looks like I forgot to change using the k8s module to kubectl like Kubespray does for everything. I'll fix this tomorrow.
@floryut @mzaian We haven't discussed writing release notes yet for the changes required in peoples inventory files. Shall I write a changelog and if so, where can I post it?

Release note are automatically generated using the PR description (release-note part)

Aha, in that case I don't think the release notes are complete enough. Can I still update the PR and alter the release notes section? Or won't that have any impact? They really need an update.

@floryut
Copy link
Member

floryut commented Apr 19, 2023

@hufhend Thanks for reporting this. Looks like I forgot to change using the k8s module to kubectl like Kubespray does for everything. I'll fix this tomorrow.
@floryut @mzaian We haven't discussed writing release notes yet for the changes required in peoples inventory files. Shall I write a changelog and if so, where can I post it?

Release note are automatically generated using the PR description (release-note part)

Aha, in that case I don't think the release notes are complete enough. Can I still update the PR and alter the release notes section? Or won't that have any impact? They really need an update.

You can update it no problem, it is used at the time we generate the release note, for now nobody as started to do that for the next release so you can update it as much as you like 👍

@Jeroen0494
Copy link
Contributor Author

@hufhend Thanks for reporting this. Looks like I forgot to change using the k8s module to kubectl like Kubespray does for everything. I'll fix this tomorrow.
@floryut @mzaian We haven't discussed writing release notes yet for the changes required in peoples inventory files. Shall I write a changelog and if so, where can I post it?

Release note are automatically generated using the PR description (release-note part)

Aha, in that case I don't think the release notes are complete enough. Can I still update the PR and alter the release notes section? Or won't that have any impact? They really need an update.

You can update it no problem, it is used at the time we generate the release note, for now nobody as started to do that for the next release so you can update it as much as you like +1

Great! I've updated the release notes and pointed to the documentation. That should be enough.

@floryut
Copy link
Member

floryut commented Apr 21, 2023

@Jeroen0494 Thank you, we'll put that in the notes with a warning 👍

@meetmatt
Copy link

@Jeroen0494 thank you for contribution. However, I've discovered a small issue: roles/kubernetes-apps/metallb/templates/metallb-config.yml.j2 was deleted, but it still remains in the template task's dict: https://github.com/kubernetes-sigs/kubespray/blob/master/roles/kubernetes-apps/metallb/tasks/main.yml#L48

@floryut
Copy link
Member

floryut commented Apr 26, 2023

@meetmatt Thank you for reporting this 🙇

@Jeroen0494
Copy link
Contributor Author

@meetmatt thanks for the report, this will be fixed in the following pr: #9995

@yankay yankay mentioned this pull request May 15, 2023
pedro-peter pushed a commit to pedro-peter/kubespray that referenced this pull request May 8, 2024
Signed-off-by: Jeroen Rijken <jeroen.rijken@xs4all.nl>
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants