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

daemon/graphdriver/windows: various cleanups and fixes #44282

Merged
merged 6 commits into from Mar 14, 2023

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Oct 10, 2022

daemon/graphdriver/windows: InitFilter() don't use idtools.MkdirAllAndChown()

idtools.MkdirAllAndChown on Windows does not chown directories, which makes
idtools.MkdirAllAndChown() just an alias for system.MkDirAll().

Also setting the filemode to 0, as changing filemode is a no-op on Windows as
well; both of these changes should make it more transparent that no chown'ing,
nor changing filemode takes place.

daemon/graphdriver/windows: rename vars that collided with imports

daemon/graphdriver/windows: remove fileFlagSequentialScan const

Replace it with the const that's now defined in golang.org/x/sys/windows

daemon/graphdriver/windows: use strings.EqualFold()

Saves some allocations

BenchmarkTolower
BenchmarkTolower-5     7917788       150.4 ns/op      16 B/op       3 allocs/op
BenchmarkEqualFold
BenchmarkEqualFold-5   8248605       143.5 ns/op       8 B/op       1 allocs/op

daemon/graphdriver/windows: remove some intermediate variables

daemon/graphdriver/windows: cleanup errors

@thaJeztah
Copy link
Member Author

@tianon @corhere ptal 🤗

daemon/graphdriver/windows/windows.go Outdated Show resolved Hide resolved
daemon/graphdriver/windows/windows.go Outdated Show resolved Hide resolved
@thaJeztah thaJeztah force-pushed the windows_filter_cleanup branch 2 times, most recently from 1f4c039 to 5b8c71a Compare January 3, 2023 12:04
@thaJeztah
Copy link
Member Author

@corhere I think I addressed your comments; PTAL

key := strings.ToLower(key)
switch key {
case "size":
// FIXME(thaJeztah): options should not be case-insensitive
Copy link
Contributor

Choose a reason for hiding this comment

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

New options should be case-sensitive, but I think we're stuck with case-folding for size for back-compat reasons unless we have a flag day.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we need to look how we can get out of this situation. Some of these came from situations where "Windows filesystem is case-insensitive, so options should be case-insensitive as well", or just "make it easier to use"). Reality is that in many cases this logic is "distributed", and in some cases may have some potential bugs lurking around. For example, in the overlay2 driver; parsing storage options is case-insensitive;

key := strings.ToLower(key)
switch key {
case "size":

But other parts in the same file doesn't take case-insensitivity into account;

// Merge daemon default config.
if _, ok := opts.StorageOpt["size"]; !ok && d.options.quota.Size != 0 {
opts.StorageOpt["size"] = strconv.FormatUint(d.options.quota.Size, 10)
}
if _, ok := opts.StorageOpt["size"]; ok && !projectQuotaSupported {
return fmt.Errorf("--storage-opt is supported only for overlay over xfs with 'pquota' mount option")
}
return d.create(id, parent, opts)
}
// Create is used to create the upper, lower, and merge directories required for overlay fs for a given id.
// The parent filesystem is used to configure these directories for the overlay.
func (d *Driver) Create(id, parent string, opts *graphdriver.CreateOpts) (retErr error) {
if opts != nil && len(opts.StorageOpt) != 0 {
if _, ok := opts.StorageOpt["size"]; ok {
return fmt.Errorf("--storage-opt size is only supported for ReadWrite Layers")
}

daemon/graphdriver/windows/windows.go Outdated Show resolved Hide resolved
Copy link
Contributor

@corhere corhere left a comment

Choose a reason for hiding this comment

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

LGTM!

@tianon
Copy link
Member

tianon commented Mar 10, 2023

Hmmmm, CI is being weird -- I think the # 2, # 3, and # 4 attempt failures are from trying to restart "only failed jobs" and that not working for some reason (thanks GitHub?) but the original failure was in the Windows tests, which feels related enough that it's maybe not the best idea for us to skip without doing a full restart? 😞

@thaJeztah
Copy link
Member Author

let me try a close/reopen to see if that runs a full cycle

@thaJeztah thaJeztah closed this Mar 12, 2023
@thaJeztah thaJeztah reopened this Mar 12, 2023
…dChown()

idtools.MkdirAllAndChown on Windows does not chown directories, which makes
idtools.MkdirAllAndChown() just an alias for system.MkDirAll().

Also setting the filemode to `0`, as changing filemode is a no-op on Windows as
well; both of these changes should make it more transparent that no chown'ing,
nor changing filemode takes place.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Replace it with the const that's now defined in golang.org/x/sys/windows

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Saves some allocations

    BenchmarkTolower
    BenchmarkTolower-5     7917788       150.4 ns/op      16 B/op       3 allocs/op
    BenchmarkEqualFold
    BenchmarkEqualFold-5   8248605       143.5 ns/op       8 B/op       1 allocs/op

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

Did a rebase to get a fresh run, and all green now 👍 bringing this in

@thaJeztah thaJeztah merged commit 889427b into moby:master Mar 14, 2023
86 checks passed
@thaJeztah thaJeztah deleted the windows_filter_cleanup branch March 14, 2023 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants