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

manager/allocator/cnmallocator: add temporary adaptor for constructor #3139

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jun 30, 2023

Add an adaptor to help with a signature change in the Idm constructor.

- What I did

- How I did it

- How to test it

- Description for the changelog

@neersighted
Copy link
Member

Looks like this has legitimate test failures hitting the "invalid constructor" branch -- but I like this idea in principle, if we can make it work...

@thaJeztah
Copy link
Member Author

Yes, looks like I messed it up, or at least didn't handle everything, but I can give it another try tomorrow

@thaJeztah thaJeztah force-pushed the cnmallocator_transition branch 4 times, most recently from f1f1a27 to e21d93d Compare July 3, 2023 15:42
Comment on lines 122 to 124
// these are deliberately aliases, otherwise we'd only match the same type, not signature.
legacyConstructor = func(ds datastore.DataStore, id string, start, end uint64) (*idm.Idm, error)
idmConstructor = func(id string, start, end uint64) (*idm.Idm, error)
Copy link
Member Author

Choose a reason for hiding this comment

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

🤦 that took me some time to understand "why it doesn't match".... got bitten by Go's strong typing;

This will only match a function that's an actual "one", not "any function with signature func(string)

type one func(string)

See below (notice that both one and three are allowed in the switch .. because, well, they're not the same:
code below will match "two", not "one": https://go.dev/play/p/L_J_To1xRhp

package main

import "fmt"

type one func(string)
type two = func(string)
type three func(string)

func myFunc(string) {}

func assertType(fn any) {
	switch f := fn.(type) {
	case one:
		fmt.Printf("one: %T == %T", f, fn)
	case two:
		fmt.Printf("two: %T == %T", f, fn)
	case three:
		fmt.Printf("three: %T == %T", f, fn)
	default:
		fmt.Printf("unknown: %T != %T", f, fn)
	}
}

func main() {
	assertType(myFunc)
}

@codecov-commenter
Copy link

Codecov Report

Merging #3139 (e21d93d) into master (93fe90a) will decrease coverage by 0.08%.
The diff coverage is 56.25%.

@@            Coverage Diff             @@
##           master    #3139      +/-   ##
==========================================
- Coverage   61.76%   61.69%   -0.08%     
==========================================
  Files         154      154              
  Lines       31106    31120      +14     
==========================================
- Hits        19214    19198      -16     
- Misses      10346    10383      +37     
+ Partials     1546     1539       -7     

thaJeztah pushed a commit to thaJeztah/docker that referenced this pull request Jul 3, 2023
testing moby/swarmkit#3139

Signed-off-by: Albin Kerouanton <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

Whoop; this is green, and my test-PR in moby with this change is also green;

@dperny @neersighted PTAL

Add an adaptor to help with a signature change in the Idm constructor.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

I think I can combine this PR with #3015, which now does something very similar for another signature change; let me update this one.

// There are no driver configurations and notification
// functions as of now.
reg, err := drvregistry.New(nil, nil, nil, nil, pg)
// FIXME(thaJeztah): drvregistry.New was deprecated in https://github.com/moby/moby/commit/5595311209cc915e8b0ace0a1bbd8b52a7baecb0, but there's no other way to pass a PluginGetter to it.
Copy link
Contributor

Choose a reason for hiding this comment

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

You're damn right there is no way to pass a PluginGetter when constructing the non-deprecated drvregistry types. That's because they don't need one. See also moby/moby@28edc8e

type (
// these are deliberately aliases, otherwise we'd only match the same type, not signature.
legacyIdmConstructor = func(ds datastore.DataStore, id string, start, end uint64) (*idm.Idm, error)
idmConstructor = func(id string, start, end uint64) (*idm.Idm, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
idmConstructor = func(id string, start, end uint64) (*idm.Idm, error)
idmConstructor = func(start, end uint64) (*idm.Idm, error)

Without the datastore, id would also be unused.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only consumers of libnetwork/idm are this file and libnetwork's ovmanager driver. Notably, ovmanager initializes its idm with start=0, which means the idm transparently delegates everything to the underlying bitmap and can therefore be trivially replaced with a bitmap.Sequence. That leaves this file as the only consumer of idm which actually needs it. Therefore I propose moving the idm package into swarmkit, which would make the adapters unnecessary.

@corhere
Copy link
Contributor

corhere commented Jul 7, 2023

This PR has been superseded by #3143

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

Successfully merging this pull request may close these issues.

4 participants