Skip to content

[image] Filter unsupported images at manifest parsing #4702

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

Open
deepakshirkem wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[image] Filter unsupported images at manifest parsing #4702

Show file tree
Hide file tree
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
3 changes: 1 addition & 2 deletions include/multipass/image_host/custom_image_host.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ class CustomVMImageHost final : public BaseVMImageHost

std::optional<VMImageInfo> info_for(const Query& query) override;
std::vector<std::pair<std::string, VMImageInfo>> all_info_for(const Query& query) override;
std::vector<VMImageInfo> all_images_for(const std::string& remote_name,
const bool allow_unsupported) override;
std::vector<VMImageInfo> all_images_for(const std::string& remote_name) override;
std::vector<std::string> supported_remotes() override;

private:
Expand Down
3 changes: 1 addition & 2 deletions include/multipass/image_host/ubuntu_image_host.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@ class UbuntuVMImageHost final : public BaseVMImageHost

std::optional<VMImageInfo> info_for(const Query& query) override;
std::vector<std::pair<std::string, VMImageInfo>> all_info_for(const Query& query) override;
std::vector<VMImageInfo> all_images_for(const std::string& remote_name,
const bool allow_unsupported) override;
std::vector<VMImageInfo> all_images_for(const std::string& remote_name) override;
std::vector<std::string> supported_remotes() override;

private:
Expand Down
3 changes: 1 addition & 2 deletions include/multipass/image_host/vm_image_host.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ class VMImageHost : private DisabledCopyMove
virtual std::optional<VMImageInfo> info_for(const Query& query) = 0;
virtual std::vector<std::pair<std::string, VMImageInfo>> all_info_for(const Query& query) = 0;
virtual VMImageInfo info_for_full_hash(const std::string& full_hash) = 0;
virtual std::vector<VMImageInfo> all_images_for(const std::string& remote_name,
const bool allow_unsupported) = 0;
virtual std::vector<VMImageInfo> all_images_for(const std::string& remote_name) = 0;
virtual void for_each_entry_do(const Action& action) = 0;
Comment on lines 40 to 43
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

VMImageHost::all_images_for signature was changed to take only remote_name, but several unit tests/mocks still use the old (remote_name, allow_unsupported) overload (e.g. tests/unit/mock_image_host.h, tests/unit/test_custom_image_host.cpp, tests/unit/test_daemon_find.cpp, tests/unit/test_ubuntu_image_host.cpp). This will break the build until those call sites and mock declarations are updated to match the new interface and expectations adjusted (unsupported images should no longer be counted/listed).

Copilot uses AI. Check for mistakes.
virtual std::vector<std::string> supported_remotes() = 0;
virtual void update_manifests(const bool force_update) = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/daemon/daemon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1663,7 +1663,7 @@ try
request->force_manifest_network_download());
const auto& remote = request->remote_name();
auto image_host = config->vault->image_host_for(remote);
auto vm_images_info = image_host->all_images_for(remote, request->allow_unsupported());
auto vm_images_info = image_host->all_images_for(remote);

for (const auto& info : vm_images_info)
add_aliases(response.mutable_images_info(), remote, info);
Expand Down
3 changes: 1 addition & 2 deletions src/image_host/custom_image_host.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,7 @@ std::vector<std::pair<std::string, mp::VMImageInfo>> mp::CustomVMImageHost::all_
return images;
}

std::vector<mp::VMImageInfo> mp::CustomVMImageHost::all_images_for(const std::string& remote_name,
const bool allow_unsupported)
std::vector<mp::VMImageInfo> mp::CustomVMImageHost::all_images_for(const std::string& remote_name)
{
if (auto custom_manifest = manifest_from(remote_name))
return custom_manifest->products;
Expand Down
18 changes: 4 additions & 14 deletions src/image_host/ubuntu_image_host.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#include <multipass/exceptions/download_exception.h>
#include <multipass/exceptions/image_not_found_exception.h>
#include <multipass/exceptions/manifest_exceptions.h>
#include <multipass/exceptions/unsupported_image_exception.h>
#include <multipass/image_host/ubuntu_image_host.h>
#include <multipass/platform.h>
#include <multipass/query.h>
Expand Down Expand Up @@ -108,9 +107,6 @@ std::vector<std::pair<std::string, mp::VMImageInfo>> mp::UbuntuVMImageHost::all_

if (const auto* info = match_alias(key, *manifest); info)
{
if (!info->supported && !query.allow_unsupported)
throw mp::UnsupportedImageException(query.release);

images.emplace_back(remote_name, *info);
}
else
Expand All @@ -119,7 +115,7 @@ std::vector<std::pair<std::string, mp::VMImageInfo>> mp::UbuntuVMImageHost::all_

for (const auto& entry : manifest->products)
{
if (entry.id.startsWith(key) && (entry.supported || query.allow_unsupported) &&
if (entry.id.startsWith(key) &&
found_hashes.find(entry.id.toStdString()) == found_hashes.end())
{
images.emplace_back(remote_name, entry);
Expand Down Expand Up @@ -148,19 +144,13 @@ mp::VMImageInfo mp::UbuntuVMImageHost::info_for_full_hash_impl(const std::string
throw mp::ImageNotFoundException(full_hash);
}

std::vector<mp::VMImageInfo> mp::UbuntuVMImageHost::all_images_for(const std::string& remote_name,
const bool allow_unsupported)
std::vector<mp::VMImageInfo> mp::UbuntuVMImageHost::all_images_for(const std::string& remote_name)
{
std::vector<mp::VMImageInfo> images;
auto manifest = manifest_from(remote_name);

for (const auto& entry : manifest->products)
{
if (entry.supported || allow_unsupported)
{
images.push_back(entry);
}
}
for (const auto& entry : manifest->products)
images.push_back(entry);

if (images.empty())
throw std::runtime_error(
Expand Down
2 changes: 2 additions & 0 deletions src/simplestreams/simple_streams_manifest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ try
const auto supported = lookup_or<bool>(product, "supported", false) ||
product_aliases.contains("devel") ||
(os == "ubuntu-core" && image_type == "stable");
if (!supported)
continue;
Comment on lines +139 to +140
Copy link
Copy Markdown
Collaborator

@ricab ricab Apr 20, 2026

Choose a reason for hiding this comment

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

Now if we're going to skip these, no point having the supported flag in VMImageInfo, right?


const auto* versions = if_contains_object(product, "versions");
if (!versions || versions->empty())
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/mock_image_host.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class MockImageHost : public VMImageHost
});
ON_CALL(*this, all_info_for(_)).WillByDefault(Return(empty_image_info_vector_pair));
ON_CALL(*this, info_for_full_hash(_)).WillByDefault(Return(empty_vm_image_info));
ON_CALL(*this, all_images_for(_, _)).WillByDefault(Return(empty_image_info_vector));
ON_CALL(*this, all_images_for(_)).WillByDefault(Return(empty_image_info_vector));
ON_CALL(*this, for_each_entry_do(_)).WillByDefault([this](const Action& action) {
action(release_remote, mock_bionic_image_info);
action(release_remote, mock_another_image_info);
Expand All @@ -95,7 +95,7 @@ class MockImageHost : public VMImageHost
MOCK_METHOD(VMImageInfo, info_for_full_hash, (const std::string&), (override));
MOCK_METHOD(std::vector<VMImageInfo>,
all_images_for,
(const std::string&, const bool),
(const std::string&),
(override));
MOCK_METHOD(void, for_each_entry_do, (const Action&), (override));
MOCK_METHOD(std::vector<std::string>, supported_remotes, (), (override));
Expand Down
3 changes: 1 addition & 2 deletions tests/unit/stub_image_host.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ struct StubVMImageHost final : public multipass::VMImageHost
return {{}, {}, {}, {}, {}, {}, {}, {}, {}, {}, -1, {}};
};

std::vector<multipass::VMImageInfo> all_images_for(const std::string& remote_name,
const bool allow_unsupported) override
std::vector<multipass::VMImageInfo> all_images_for(const std::string& remote_name) override
{
return {};
};
Expand Down
14 changes: 7 additions & 7 deletions tests/unit/test_custom_image_host.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ TEST_F(CustomImageHost, allImagesForNoRemoteReturnsAppropriateMatches)

host.update_manifests(false);

auto images = host.all_images_for("", false);
auto images = host.all_images_for("");
int supported_count = num_images_for_arch(payload);

EXPECT_EQ(images.size(), supported_count);
Expand Down Expand Up @@ -166,11 +166,11 @@ TEST_F(CustomImageHost, handlesAndRecoversFromInitialNetworkFailure)
int supported_count = num_images_for_arch(payload);

host.update_manifests(false);
auto images_info = host.all_images_for("", false);
auto images_info = host.all_images_for("");
EXPECT_EQ(images_info.size(), 0);

host.update_manifests(false);
images_info = host.all_images_for("", false);
images_info = host.all_images_for("");
EXPECT_EQ(images_info.size(), supported_count);
}

Expand All @@ -185,13 +185,13 @@ TEST_F(CustomImageHost, handlesAndRecoversFromLaterNetworkFailure)
int supported_count = num_images_for_arch(payload);

host.update_manifests(false);
EXPECT_EQ(host.all_images_for("", false).size(), supported_count);
EXPECT_EQ(host.all_images_for("").size(), supported_count);

host.update_manifests(false);
EXPECT_EQ(host.all_images_for("", false).size(), 0);
EXPECT_EQ(host.all_images_for("").size(), 0);

host.update_manifests(false);
EXPECT_EQ(host.all_images_for("", false).size(), supported_count);
EXPECT_EQ(host.all_images_for("").size(), supported_count);
}

TEST_F(CustomImageHost, infoForFullHashReturnsEmptyImageInfo)
Expand Down Expand Up @@ -238,7 +238,7 @@ TEST_F(CustomImageHost, badJsonLogsAndReturnsEmptyImages)

host.update_manifests(false);

auto images = host.all_images_for("", false);
auto images = host.all_images_for("");

EXPECT_EQ(images.size(), 0);
}
2 changes: 1 addition & 1 deletion tests/unit/test_daemon_find.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ TEST_F(DaemonFind, forByRemoteReturnsExpectedData)
return &mock_image_host;
});

EXPECT_CALL(mock_image_host, all_images_for(_, _)).WillOnce([&mock_image_host](auto...) {
EXPECT_CALL(mock_image_host, all_images_for(_)).WillOnce([&mock_image_host](auto...) {
std::vector<mp::VMImageInfo> images_info;

images_info.push_back(mock_image_host.mock_bionic_image_info);
Expand Down
52 changes: 52 additions & 0 deletions tests/unit/test_data/simple_streams_manifest/unsupported_manifest.json.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
{
"content_id": "com.ubuntu.cloud:released:download",
"datatype": "image-downloads",
"format": "products:1.0",
"updated": "Wed, 20 May 2020 16:47:50 +0000",
"products": {
"com.ubuntu.cloud:server:16.04:@MANIFEST_ARCH@": {
"aliases": "16.04,xenial",
"arch": "@MANIFEST_ARCH@",
"os": "ubuntu",
"release": "xenial",
"release_codename": "Xenial Xerus",
"release_title": "16.04 LTS",
"supported": false,
"version": "16.04",
"versions": {
"20170516": {
"items": {
"disk1.img": {
"ftype": "disk1.img",
"path": "server/releases/xenial/release-20170516/ubuntu-16.04-server-cloudimg-@MANIFEST_ARCH@-disk1.img",
"sha256": "1797c5c82016c1e65f4008fcf89deae3a044ef76087a9ec5b907c6d64a3609ac",
"size": 287440896
}
}
}
}
},
"com.ubuntu.cloud:server:18.04:@MANIFEST_ARCH@": {
"aliases": "18.04,bionic,default",
"arch": "@MANIFEST_ARCH@",
"os": "ubuntu",
"release": "bionic",
"release_codename": "Bionic Beaver",
"release_title": "18.04 LTS",
"supported": true,
"version": "18.04",
"versions": {
"20200518": {
"items": {
"disk1.img": {
"ftype": "disk1.img",
"path": "server/releases/bionic/release-20200518/ubuntu-18.04-server-cloudimg-@MANIFEST_ARCH@.img",
"sha256": "19f9b706755717ee7b38a4a79d8e7599e3fd96d57fb824b8e68ac92255a8323d",
"size": 345505792
}
}
}
}
}
}
}
35 changes: 3 additions & 32 deletions tests/unit/test_image_vault.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
#include <multipass/exceptions/aborted_download_exception.h>
#include <multipass/exceptions/create_image_exception.h>
#include <multipass/exceptions/image_vault_exceptions.h>
#include <multipass/exceptions/unsupported_image_exception.h>
#include <multipass/format.h>
#include <multipass/platform.h>
#include <multipass/query.h>
Expand Down Expand Up @@ -1224,7 +1223,7 @@ TEST_F(ImageVault, allInfoForNoRemoteGivenReturnsExpectedData)
{remote_name, host.mock_bionic_image_info},
{remote_name, host.mock_another_image_info}}));

auto images = vault.all_info_for({"", "e3", false, "", mp::Query::Type::Alias, true});
auto images = vault.all_info_for({"", "e3", false, "", mp::Query::Type::Alias});

EXPECT_EQ(images.size(), 2u);

Expand Down Expand Up @@ -1254,7 +1253,7 @@ TEST_F(ImageVault, allInfoForRemoteGivenReturnsExpectedData)
{remote_name, host.mock_bionic_image_info},
{remote_name, host.mock_another_image_info}}));

auto images = vault.all_info_for({"", "e3", false, remote_name, mp::Query::Type::Alias, true});
auto images = vault.all_info_for({"", "e3", false, remote_name, mp::Query::Type::Alias});

EXPECT_EQ(images.size(), 2u);

Expand Down Expand Up @@ -1282,35 +1281,7 @@ TEST_F(ImageVault, allInfoForNoImagesReturnsEmpty)
EXPECT_CALL(host, all_info_for(_))
.WillOnce(Return(std::vector<std::pair<std::string, mp::VMImageInfo>>{}));

EXPECT_TRUE(vault.all_info_for({"", name, false, "", mp::Query::Type::Alias, true}).empty());
}

TEST_F(ImageVault, updateImagesLogsWarningOnUnsupportedImage)
{
mpt::MockLogger::Scope logger_scope = mpt::MockLogger::inject(mpl::Level::warning);
mp::DefaultVMImageVault vault{hosts,
&url_downloader,
cache_dir.path(),
data_dir.path(),
mp::days{1}};
vault.fetch_image(mp::FetchType::ImageOnly,
default_query,
stub_prepare,
stub_monitor,
std::nullopt,
instance_dir);

EXPECT_CALL(host, info_for(_))
.WillOnce(Throw(mp::UnsupportedImageException(default_query.release)));

logger_scope.mock_logger->screen_logs(mpl::Level::warning);
EXPECT_CALL(*logger_scope.mock_logger,
log(mpl::Level::warning,
StrEq("image vault"),
StrEq(fmt::format("Skipping update: The {} release is no longer supported.",
default_query.release))));

EXPECT_NO_THROW(vault.update_images(mp::FetchType::ImageOnly, stub_prepare, stub_monitor));
EXPECT_TRUE(vault.all_info_for({"", name, false, "", mp::Query::Type::Alias}).empty());
}

TEST_F(ImageVault, updateImagesLogsWarningOnEmptyVault)
Expand Down
18 changes: 17 additions & 1 deletion tests/unit/test_simple_streams_manifest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,4 +204,20 @@ TEST_F(TestSimpleStreamsManifest, correctlyMutatesCoreImages)
EXPECT_EQ(info->release_codename, "Core 22");
}

} // namespace
TEST_F(TestSimpleStreamsManifest, filtersUnsupportedImages)
{
auto json = mpt::load_test_file("simple_streams_manifest/unsupported_manifest.json");
auto manifest = mp::SimpleStreamsManifest::fromJson(json, std::nullopt, "");

EXPECT_THAT(manifest->products.size(), Eq(1u));

auto info = manifest->image_records.find("bionic");
EXPECT_NE(info, manifest->image_records.end());

info = manifest->image_records.find("xenial");
EXPECT_EQ(info, manifest->image_records.end());

info = manifest->image_records.find("16.04");
EXPECT_EQ(info, manifest->image_records.end());
}
} // namespace
Copy link
Copy Markdown
Collaborator

@ricab ricab Apr 20, 2026

Choose a reason for hiding this comment

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

Missing EOL.

Loading