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

Add option to append mirror configuration instead of overwriting #424

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

phillebaba
Copy link
Member

This change adds a new option to append to existing mirror configuration, which is useful when using Spegel together with other mirrors.

The appending will be done to the contents in the backup directory. As files are only backed up the first time Spegel runs it means that the original mirror configuration will be persisted and reconfigured every time Spegel restarts. The feature is disabled by default to keep the original behavior.

Fixes #413

pkg/oci/containerd.go Outdated Show resolved Hide resolved
Copy link
Contributor

@bittrance bittrance left a comment

Choose a reason for hiding this comment

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

This looks reasonable. The central code is a little hard to follow, see review comment for suggestion.

}
log.Info("appending to existing Containerd mirror configuration", "registry", registryURL.String(), "path", fp)
}
}
Copy link
Contributor

@bittrance bittrance Apr 8, 2024

Choose a reason for hiding this comment

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

I think I would have formulated this as:

hf := hostFile{
	Server:      server,
	HostConfigs: map[string]hostConfig{},
}
backupUsed := false
if appendMirrors {
	var err
	fp := path.Join(configPath, backupDir, registryURL.Host, "hosts.toml")
	hf, backupUsed, err = loadBackupHostFile(fs, fp)
	if err != nil {
		return err
	}
}

Using backupUsed we can then generate separate log messages for the separate cases rather than generating an extra log message in the append case.

This leads to three improvements (imo):

  • it is clear we read backups from the helper name and the parameter can retain the nameappendMirrors instead of switching to appnedToBackup, and
  • it is easier to see that we are replacing hf in the append case, and
  • denser log output

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 totally agree with you.

I had the intention of refactoring this code at a later time when implementing the CRIO version. After some trouble understanding my own code I chose to refactor it now. Added your suggestion to reduce the amount of logs.

@phillebaba phillebaba force-pushed the feature/mirror-append branch 2 times, most recently from c203a4d to ee0e692 Compare April 8, 2024 19:19
@phillebaba phillebaba merged commit 104150e into main Apr 9, 2024
8 checks passed
@phillebaba phillebaba deleted the feature/mirror-append branch April 9, 2024 15:04
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.

Removal of certs in containerd configuration causing 500 error (mirror resolve retries exhausted for key)
2 participants