Skip to content

storage: fix private templates are not copied to new image store #9206

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.

Sign up for GitHub

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

Merged
yadvr merged 1 commit into from
Sep 4, 2024
Merged

storage: fix private templates are not copied to new image store #9206

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions .../storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,11 +532,6 @@ public void handleTemplateSync(DataStore store) {
s_logger.info("Skip downloading template " + tmplt.getUniqueName() + " since no url is specified.");
continue;
}
// if this is private template, skip sync to a new image store
if (isSkipTemplateStoreDownload(tmplt, zoneId)) {
Copy link
Contributor

@shwstppr shwstppr Jun 11, 2024

Choose a reason for hiding this comment

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

I feel the log here is probably wrong otherwise the last condition in the method should take care of the case where the template is private but it needs to be downloaded to a new zone? Or do we download all templates to all stores of the zones?

Copy link
Member Author

@weizhouapache weizhouapache Jun 12, 2024

Choose a reason for hiding this comment

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

the method isSkipTemplateStoreDownload was added in your pr #7485

the original code snippet was introduced many years ago, only public/featured/system templates are copied.

                            if (!tmplt.isPublicTemplate() && !tmplt.isFeatured() && tmplt.getTemplateType() != TemplateType.SYSTEM) {
                                s_logger.info("Skip sync downloading private template " + tmplt.getUniqueName() + " to a new image store");
                                continue;
                            }

in my opinion, all the templates/isos should be copied to new image store.
this needs some discussion
(this is why I created this pr as draft. isSkipTemplateStoreDownload can be removed later in this pr)

Copy link
Contributor

@DaanHoogland DaanHoogland Jun 21, 2024

Choose a reason for hiding this comment

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

will removing this make private templates be copied cross zones? If it is intra zone yes it makes sense to have templates on all stores, though ACS should be smart enough to do with one copy IMNSHO. I think we can remove this check now and consider making template retrieval smarter later.

Copy link
Contributor

@sureshanaparti sureshanaparti Jun 24, 2024

Choose a reason for hiding this comment

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

@weizhouapache Can this be controlled through a config (which can be default to false - keep the existing behavior as well)?

Copy link
Member Author

@weizhouapache weizhouapache Jun 24, 2024

Choose a reason for hiding this comment

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

@sureshanaparti @DaanHoogland
to be frank, I see no reason why private templates cannot be copied to other zones.
the public/private property is only related to user access control (domain/account), not the locations (zones/regions).
I prefer to consider it as a bug.

Copy link
Contributor

@DaanHoogland DaanHoogland Jul 1, 2024

Choose a reason for hiding this comment

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

@weizhouapache , I could live with that but would consider it a gap in functional definition. It was intentionally prohibited in the original code, but I see your point why that would not make sense. Let's go forward with this fix. In this case a setting is not a blocker IMNSHO. We can do but can do without as well. cc @sureshanaparti @weizhouapache

s_logger.info("Skip sync downloading private template " + tmplt.getUniqueName() + " to a new image store");
continue;
}

// if this is a region store, and there is already an DOWNLOADED entry there without install_path information, which
// means that this is a duplicate entry from migration of previous NFS to staging.
Expand Down