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

Update zfs-driver.md (remove section "Limit a container's writable storage quota") #17828

Closed
wants to merge 1 commit into from

Conversation

Harliff
Copy link

@Harliff Harliff commented Jul 26, 2023

Instruction how to limit container's size has been removed because this option doesn't work and prevent the docker service from starting up.

See moby/moby#33847 for bug description

Proposed changes

I've removed the section of docker documentation since it contains wrong instruction.

Related issues (optional)

(moby/moby#33847)

Instruction how to limit container's size has been removed because this option doesn't work and prevent the docker service from starting up.

See moby/moby#33847 for bug description
@netlify
Copy link

netlify bot commented Jul 26, 2023

Deploy Preview for docsdocker ready!

Name Link
🔨 Latest commit 09677f8
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/64c133e8d2f63d0008ed9e35
😎 Deploy Preview https://deploy-preview-17828--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@thaJeztah
Copy link
Member

Hm... interesting; I need to check this one; looking for changes related to this, I found a pull-request that implemented the size option for zfs;

However, the storage-driver's options validation does not include the size option, so either that feature was reverted, or some other change made a mistake in the option-validation; https://github.com/moby/moby/blob/7a44e4cde0b85c81642aac69f7a5d5fe76ea86f4/daemon/graphdriver/zfs/zfs.go#L127-L144

func parseOptions(opt []string) (zfsOptions, error) {
	var options zfsOptions
	options.fsName = ""
	for _, option := range opt {
		key, val, err := parsers.ParseKeyValueOpt(option)
		if err != nil {
			return options, err
		}
		key = strings.ToLower(key)
		switch key {
		case "zfs.fsname":
			options.fsName = val
		default:
			return options, fmt.Errorf("Unknown option %s", key)
		}
	}
	return options, nil
}

@thaJeztah
Copy link
Member

DOH! Looking at parseOptions, not parseStorageOpt, which still has the size option; https://github.com/moby/moby/blob/7a44e4cde0b85c81642aac69f7a5d5fe76ea86f4/daemon/graphdriver/zfs/zfs.go#L324-L336

func parseStorageOpt(storageOpt map[string]string) (string, error) {
	// Read size to change the disk quota per container
	for k, v := range storageOpt {
		key := strings.ToLower(k)
		switch key {
		case "size":
			return v, nil
		default:
			return "0", fmt.Errorf("Unknown option %s", key)
		}
	}
	return "0", nil
}

@craig-osterhout craig-osterhout added the area/storage Relates to storage, volumes label Jul 26, 2023
@dvdksn
Copy link
Collaborator

dvdksn commented Aug 24, 2023

@thaJeztah should this be closed then?

@docker-robot
Copy link

docker-robot bot commented Nov 22, 2023

Thanks for the pull request. We'd like to make our product docs better, but haven’t been able to review all the suggestions.
As our docs have also diverged, we do not have the bandwidth to review and rebase old pull requests.

If the updates are still relevant, review our contribution guidelines and rebase your pull request against the latest version of the docs, then mark it as fresh with a /remove-lifecycle stale comment.
If not, this pull request will be closed in 30 days. This helps our maintainers focus on the active pull requests.

Prevent pull requests from auto-closing with a /lifecycle frozen comment.

/lifecycle stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/storage Relates to storage, volumes lifecycle/stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants