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

fix(machine): Remove link from store if already deleted #832

Draft
wants to merge 2 commits into
base: staging
Choose a base branch
from

Conversation

craciunoiuc
Copy link
Member

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran make fmt on your commit series before opening this PR;
  • Updated relevant documentation.

Description of changes

This fixes the case where someone creates a network with kraftkit and removes it with something else like ifconfig/ip link del and the entry stays in the list.

Partial fix for: #820

@craciunoiuc craciunoiuc added the kind/fix This PR fixes an issue or bug label Sep 20, 2023
@craciunoiuc craciunoiuc force-pushed the craciunoiuc/remove-missing-links branch 3 times, most recently from 2c78437 to 68a2551 Compare September 20, 2023 12:47
Comment on lines +363 to +377
embeddedStore, err := store.NewEmbeddedStore[networkv1alpha1.NetworkSpec, networkv1alpha1.NetworkStatus](
filepath.Join(
config.G[config.KraftKit](ctx).RuntimeDir,
"networkv1alpha1",
),
)
if err != nil {
return nil, err
}

var existingNetwork networkv1alpha1.Network
err = embeddedStore.Get(ctx, network.Name, storage.GetOptions{}, &existingNetwork)
if err != nil && !strings.Contains(err.Error(), "not found") {
return nil, err
}
Copy link
Member

Choose a reason for hiding this comment

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

The store is already wrapping the v1alpha1 implementation, meaning that this obfuscates its use.

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 think the wrapping itself makes everything feel obfuscated, and it's fine. As long as it works correctly >:D

Copy link
Member

@nderjung nderjung left a comment

Choose a reason for hiding this comment

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

In fact the Delete method should not be updated. The Get/List method should be checked to ensure they only return valid entries from the host.

@craciunoiuc
Copy link
Member Author

Ok then the fix would be to remove from the store on list?

@craciunoiuc
Copy link
Member Author

It will still fail if you run ip link del + kraft net rm without kraft net ls in between

@nderjung
Copy link
Member

nderjung commented Sep 20, 2023

When kraft net ls is run, eventually the network controller/v1alpha1 implemention will be called which calls List. This prototype will receive a "stale" list of networks in the positional parameter networks:

func (service *v1alpha1Network) List(ctx context.Context, networks *networkv1alpha1.NetworkList) (*networkv1alpha1.NetworkList, error) {
                                                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The value of networks was populated by going through the store. During the iteration of this list, items need to be added and removed based on whether they exist or not. Clearly this is not working correctly here.

The result List should return an up-to-date list of networks which then is passed back to the store and saved.

The store essentially provides us with "enriched" metadata about items.

@craciunoiuc
Copy link
Member Author

Ok I can try to fix there. Hopefully the store is also updated by the returned value, otherwise things will never be removed there

@craciunoiuc craciunoiuc force-pushed the craciunoiuc/remove-missing-links branch 3 times, most recently from 29c6884 to 68a2551 Compare October 27, 2023 07:57
@craciunoiuc craciunoiuc force-pushed the craciunoiuc/remove-missing-links branch from 68a2551 to 8d77118 Compare October 30, 2023 14:51
Signed-off-by: Cezar Craciunoiu <[email protected]>
@craciunoiuc craciunoiuc force-pushed the craciunoiuc/remove-missing-links branch from 8d77118 to cf6c78b Compare October 30, 2023 15:31
@craciunoiuc
Copy link
Member Author

Converting to draft as not really ready

The bug is still there by the way

@craciunoiuc craciunoiuc marked this pull request as draft December 19, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/fix This PR fixes an issue or bug
Projects
Status: 🧊 Icebox
Development

Successfully merging this pull request may close these issues.

2 participants