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

incusd/instances/publish: Fix base metadata #1374

Merged
merged 4 commits into from
Nov 14, 2024
Merged

Conversation

stgraber
Copy link
Member

Closes #1371

@Salamandar
Copy link

Salamandar commented Nov 14, 2024

There actually is a bug that prevents the re-writing of metadata.yaml. We should not test for this and always rewrite the file:
stgraber@12b7b37#diff-33b72d6f2eb9bdf746e65cf613e9bf838ef1bafc1a576e2667c7ef4f7899de2cR5294

EDIT: The same test is performed to conditionnally add the file to the archive (well it conditions the offset and the second branch is buggy) and should be removed too:
stgraber@12b7b37#diff-33b72d6f2eb9bdf746e65cf613e9bf838ef1bafc1a576e2667c7ef4f7899de2cR5294

@Salamandar
Copy link

The whole if/else block could be replaced by :


	if util.PathExists(fnam) {
		logger.Errorf("metadata.yaml DID exist")
		// Parse the metadata.
		content, err := os.ReadFile(fnam)
		if err != nil {
			_ = tarWriter.Close()
			d.logger.Error("Failed exporting instance", ctxMap)
			return meta, err
		}

		err = yaml.Unmarshal(content, &meta)
		if err != nil {
			_ = tarWriter.Close()
			d.logger.Error("Failed exporting instance", ctxMap)
			return meta, err
		}

		// Fill in the metadata.
		meta.Architecture = arch
		meta.CreationDate = time.Now().UTC().Unix()
		if !expiration.IsZero() {
			meta.ExpiryDate = expiration.UTC().Unix()
		}
	}

	// Fill in the metadata.
	meta.Architecture = arch
	meta.CreationDate = time.Now().UTC().Unix()
	meta.Properties = properties
	if !expiration.IsZero() {
		meta.ExpiryDate = expiration.UTC().Unix()
	}

	// Generate a new metadata.yaml.
	tempDir, err := os.MkdirTemp("", "incus_metadata_")
	if err != nil {
		_ = tarWriter.Close()
		d.logger.Error("Failed exporting instance", ctxMap)
		return meta, err
	}

	defer func() { _ = os.RemoveAll(tempDir) }()

	data, err := yaml.Marshal(&meta)
	if err != nil {
		_ = tarWriter.Close()
		d.logger.Error("Failed exporting instance", ctxMap)
		return meta, err
	}

	// Write the actual file.
	fnam = filepath.Join(tempDir, "metadata.yaml")
	err = os.WriteFile(fnam, data, 0644)
	if err != nil {
		_ = tarWriter.Close()
		d.logger.Error("Failed exporting instance", ctxMap)
		return meta, err
	}

	// Include metadata.yaml in the tarball.
	fi, err := os.Lstat(fnam)
	if err != nil {
		_ = tarWriter.Close()
		d.logger.Debug("Error statting during export", logger.Ctx{"fileName": fnam})
		d.logger.Error("Failed exporting instance", ctxMap)
		return meta, err
	}

	tmpOffset := len(path.Dir(fnam)) + 1
	err = tarWriter.WriteFile(fnam[tmpOffset:], fnam, fi, false)
	if err != nil {
		_ = tarWriter.Close()
		d.logger.Debug("Error writing to tarfile", logger.Ctx{"err": err})
		d.logger.Error("Failed exporting instance", ctxMap)
		return meta, err
	}
	

That way we can ensure the creationDate, expiry, properties are all overwritten in all cases, as expected.

@stgraber
Copy link
Member Author

We shouldn't rewrite the properties, priorities should only be generated if the metadata doesn't already exist as the user may have properly pre-created/updated the metadata through incus config metadata edit.

@stgraber stgraber force-pushed the publish branch 2 times, most recently from e1aa98f to aa4c961 Compare November 14, 2024 18:23
@hallyn hallyn merged commit e019bb2 into lxc:main Nov 14, 2024
30 checks passed
@Salamandar
Copy link

Alright, looks like the state of the current main branch fixes the issue. Thanks a lot ! <3

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

Successfully merging this pull request may close these issues.

incus image export puts wrong creation_date
3 participants