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.

Sign up for GitHub

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

Jump to bottom

feat: added persistent-mac option #1829

Merged
merged 1 commit into from
Dec 21, 2023
Merged

feat: added persistent-mac option #1829

merged 1 commit into from
Dec 21, 2023

Conversation

siredmar
Copy link
Contributor

@siredmar siredmar commented Nov 23, 2023 •
edited
Loading

Description

fixes #1828

New feature: flag persistent-mac that reads the nodes annotation during startup and spins up the flannel.1 interface with the same MAC address. This can be useful when a reboot of the node happened but the old setup needs to be restored.

Release Note

The MAC address of the flannel interface is now persistent beyond restarts

@siredmar
Copy link
Contributor Author

siredmar commented Dec 6, 2023

@thomasferrandiz @rbrtbnfgl does any of you guys have time to review this?

@rbrtbnfgl
Copy link
Contributor

Hi sorry I was on vacation the last few weeks. Why adding a flag on the cmdline to enable this? Could it be enough to only check the annotation and if it's configured it'll use the defined MAC?

@siredmar
Copy link
Contributor Author

siredmar commented Dec 7, 2023

@rbrtbnfgl no problem :)
Sure, this would also be possible but would break current behavior.
If the new randomized MAC address after a flannel restart isn't by intention or not important at all this would be a nice solution.
What do you think?

@thomasferrandiz
Copy link
Contributor

One important thing to note is that the k8s client should not be imported in main.go.
All the k8s-related code should be in the kube package (https://github.com/flannel-io/flannel/tree/master/pkg/subnet/kube) since flannel can be used k8s.

@siredmar
Copy link
Contributor Author

siredmar commented Dec 8, 2023 •
edited
Loading

@thomasferrandiz moved k8s related code
@rbrtbnfgl removed persistent-flag argument. flannel reads node annotations, if the annotation is present the MAC is used.

@siredmar
Copy link
Contributor Author

siredmar commented Dec 8, 2023

@thomasferrandiz @rbrtbnfgl not sure why the k3s e2e test fails... do you have any idea why?

@thomasferrandiz
Copy link
Contributor

I restarted the test and it's fine now.
It tends to fail sometimes, it's not related to your PR.

@siredmar
Copy link
Contributor Author

siredmar commented Dec 9, 2023

would you like to take a look again so we can merge this?

manuelbuil
manuelbuil requested changes Dec 18, 2023
View reviewed changes
Copy link
Collaborator

@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.

By reading the release notes, it seems there is going to be a new flag, but I don't see it in the code.

Could you add in the docs how user could operate this new feature please? Thanks!

@rbrtbnfgl
Copy link
Contributor

I suggested to remove the flag. The feature should be enabled by default and it shouldn't break anything theoretically.

manuelbuil
manuelbuil requested changes Dec 18, 2023
View reviewed changes
pkg/subnet/kube/kube.go Outdated
// The kube subnet mgr needs to know the k8s node name that it's running on so it can annotate it.
// If we're running as a pod then the POD_NAME and POD_NAMESPACE will be populated and can be used to find the node
// name. Otherwise, the environment variable NODE_NAME can be passed in.
nodeName := os.Getenv("NODE_NAME")
Copy link
Collaborator

Choose a reason for hiding this comment

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

kubeSubnetManager struct already includes the node (nodeName field), can't you use that one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are absolutely right!

pkg/subnet/kube/kube.go
@@ -160,6 +162,7 @@ func NewSubnetManager(ctx context.Context, apiUrl, kubeconfig, prefix, netConfPa
func newKubeSubnetManager(ctx context.Context, c clientset.Interface, sc *subnet.Config, nodeName, prefix string, useMultiClusterCidr bool) (*kubeSubnetManager, error) {
var err error
var ksm kubeSubnetManager
ksm.annotationPrefix = prefix
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need this? ksm.annotations is already included the prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to store the prefix that is passed newKubeSubnetManager in order to parse the annotations for the stored MAC address. Otherwise i would need to look at ksm.annotations and split any of them to get the prefix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, you have a point. Actually, it is strange that we allow to configure this

@siredmar siredmar requested a review from manuelbuil December 19, 2023 06:40
@siredmar
Copy link
Contributor Author

By reading the release notes, it seems there is going to be a new flag, but I don't see it in the code.

Could you add in the docs how user could operate this new feature please? Thanks!

I changed the release notes

@siredmar
Copy link
Contributor Author

@manuelbuil the e2e test failed again. Would you be so kind to restart it?

@thomasferrandiz
Copy link
Contributor

@siredmar could you squash the commits please?
Then we can merge the PR

@siredmar
Copy link
Contributor Author

@thomasferrandiz done

@thomasferrandiz thomasferrandiz merged commit c051105 into flannel-io:master Dec 21, 2023
8 checks passed
@siredmar
Copy link
Contributor Author

Thx

@siredmar siredmar deleted the persistentMAC branch December 21, 2023 11:49
@manuelbuil manuelbuil mentioned this pull request Apr 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasferrandiz thomasferrandiz thomasferrandiz approved these changes

@manuelbuil manuelbuil manuelbuil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Persistent MAC address for flannel.1 beyond reboots
4 participants
@siredmar @rbrtbnfgl @thomasferrandiz @manuelbuil