From ba8d566fb6d10544b46c43a80370d1f7a2e6c2dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Graber?= Date: Wed, 25 Sep 2024 09:51:32 -0400 Subject: [PATCH 1/2] incusd: Only emit image-created if an image was actually created MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stéphane Graber --- cmd/incusd/daemon_images.go | 90 ++++++++++++++++++------------------ cmd/incusd/images.go | 6 +-- cmd/incusd/instances_post.go | 16 ++++--- 3 files changed, 57 insertions(+), 55 deletions(-) diff --git a/cmd/incusd/daemon_images.go b/cmd/incusd/daemon_images.go index beb0e1b7f55..96ff5d2bcdb 100644 --- a/cmd/incusd/daemon_images.go +++ b/cmd/incusd/daemon_images.go @@ -58,7 +58,7 @@ func imageOperationLock(ctx context.Context, fingerprint string) (locking.Unlock } // ImageDownload resolves the image fingerprint and if not in the database, downloads it. -func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *operations.Operation, args *ImageDownloadArgs) (*api.Image, error) { +func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *operations.Operation, args *ImageDownloadArgs) (*api.Image, bool, error) { var err error var ctxMap logger.Ctx @@ -91,7 +91,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope // Setup client remote, err = incus.ConnectPublicIncus(args.Server, clientArgs) if err != nil { - return nil, fmt.Errorf("Failed to connect to the server %q: %w", args.Server, err) + return nil, false, fmt.Errorf("Failed to connect to the server %q: %w", args.Server, err) } server, ok := remote.(incus.InstanceServer) @@ -102,13 +102,13 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope // Setup OCI client remote, err = incus.ConnectOCI(args.Server, clientArgs) if err != nil { - return nil, fmt.Errorf("Failed to connect to simple streams server %q: %w", args.Server, err) + return nil, false, fmt.Errorf("Failed to connect to simple streams server %q: %w", args.Server, err) } } else if protocol == "simplestreams" { // Setup simplestreams client remote, err = incus.ConnectSimpleStreams(args.Server, clientArgs) if err != nil { - return nil, fmt.Errorf("Failed to connect to simple streams server %q: %w", args.Server, err) + return nil, false, fmt.Errorf("Failed to connect to simple streams server %q: %w", args.Server, err) } } @@ -123,7 +123,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope // Expand partial fingerprints info, _, err = remote.GetImage(fp) if err != nil { - return nil, fmt.Errorf("Failed getting remote image info: %w", err) + return nil, false, fmt.Errorf("Failed getting remote image info: %w", err) } fp = info.Fingerprint @@ -133,7 +133,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope // Ensure we are the only ones operating on this image. unlock, err := imageOperationLock(ctx, fp) if err != nil { - return nil, err + return nil, false, err } defer unlock() @@ -158,7 +158,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope return nil }) if err != nil { - return nil, err + return nil, false, err } } @@ -180,14 +180,14 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope return err }) if err != nil { - return nil, fmt.Errorf("Failed locating image %q in the cluster: %w", imgInfo.Fingerprint, err) + return nil, false, fmt.Errorf("Failed locating image %q in the cluster: %w", imgInfo.Fingerprint, err) } if nodeAddress != "" { // The image is available from another node, let's try to import it. err = instanceImageTransfer(s, r, args.ProjectName, imgInfo.Fingerprint, nodeAddress) if err != nil { - return nil, fmt.Errorf("Failed transferring image %q from %q: %w", imgInfo.Fingerprint, nodeAddress, err) + return nil, false, fmt.Errorf("Failed transferring image %q from %q: %w", imgInfo.Fingerprint, nodeAddress, err) } err = s.DB.Cluster.Transaction(ctx, func(ctx context.Context, tx *db.ClusterTx) error { @@ -195,7 +195,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope return tx.AddImageToLocalNode(ctx, args.ProjectName, imgInfo.Fingerprint) }) if err != nil { - return nil, fmt.Errorf("Failed adding transferred image %q to local cluster member: %w", imgInfo.Fingerprint, err) + return nil, false, fmt.Errorf("Failed adding transferred image %q to local cluster member: %w", imgInfo.Fingerprint, err) } } } else if response.IsNotFoundError(err) { @@ -240,7 +240,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope return tx.CreateImageSource(ctx, id, args.Server, args.Protocol, args.Certificate, alias) }) if err != nil { - return nil, err + return nil, false, err } // Transfer image if needed (after database record has been created above). @@ -248,7 +248,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope // The image is available from another node, let's try to import it. err = instanceImageTransfer(s, r, args.ProjectName, info.Fingerprint, nodeAddress) if err != nil { - return nil, fmt.Errorf("Failed transferring image: %w", err) + return nil, false, fmt.Errorf("Failed transferring image: %w", err) } } } @@ -261,7 +261,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope // If not requested in a particular pool, we're done. if args.StoragePool == "" { - return info, nil + return info, false, nil } ctxMap["pool"] = args.StoragePool @@ -282,12 +282,12 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope return err }) if err != nil { - return nil, err + return nil, false, err } if slices.Contains(poolIDs, poolID) { logger.Debug("Image already exists on storage pool", ctxMap) - return info, nil + return info, false, nil } // Import the image in the pool. @@ -297,11 +297,11 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope if err != nil { ctxMap["err"] = err logger.Debug("Failed to create image on storage pool", ctxMap) - return nil, fmt.Errorf("Failed to create image %q on storage pool %q: %w", info.Fingerprint, args.StoragePool, err) + return nil, false, fmt.Errorf("Failed to create image %q on storage pool %q: %w", info.Fingerprint, args.StoragePool, err) } logger.Debug("Created image on storage pool", ctxMap) - return info, nil + return info, false, nil } // Begin downloading @@ -353,14 +353,14 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope // Create the target files dest, err := os.Create(destName) if err != nil { - return nil, err + return nil, false, err } defer func() { _ = dest.Close() }() destRootfs, err := os.Create(destName + ".rootfs") if err != nil { - return nil, err + return nil, false, err } defer func() { _ = destRootfs.Close() }() @@ -370,7 +370,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope if args.Secret != "" { info, _, err = remote.GetPrivateImage(fp, args.Secret) if err != nil { - return nil, err + return nil, false, err } // Expand the fingerprint now and mark alias string to match @@ -379,7 +379,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope } else { info, _, err = remote.GetImage(fp) if err != nil { - return nil, err + return nil, false, err } } } @@ -390,7 +390,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope } if args.Budget > 0 && info.Size > args.Budget { - return nil, fmt.Errorf("Remote image with size %d exceeds allowed bugdget of %d", info.Size, args.Budget) + return nil, false, fmt.Errorf("Remote image with size %d exceeds allowed bugdget of %d", info.Size, args.Budget) } // Download the image @@ -417,44 +417,44 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope } if err != nil { - return nil, err + return nil, false, err } // Truncate down to size if resp.RootfsSize > 0 { err = destRootfs.Truncate(resp.RootfsSize) if err != nil { - return nil, err + return nil, false, err } } err = dest.Truncate(resp.MetaSize) if err != nil { - return nil, err + return nil, false, err } // Deal with unified images if resp.RootfsSize == 0 { err := os.Remove(destName + ".rootfs") if err != nil { - return nil, err + return nil, false, err } } err = dest.Close() if err != nil { - return nil, err + return nil, false, err } err = destRootfs.Close() if err != nil { - return nil, err + return nil, false, err } } else if protocol == "direct" { // Setup HTTP client httpClient, err := localUtil.HTTPClient(args.Certificate, s.Proxy) if err != nil { - return nil, err + return nil, false, err } // Use relatively short response header timeout so as not to hold the image lock open too long. @@ -463,7 +463,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope req, err := http.NewRequest("GET", args.Server, nil) if err != nil { - return nil, err + return nil, false, err } req.Header.Set("User-Agent", version.UserAgent) @@ -471,13 +471,13 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope // Make the request raw, doneCh, err := cancel.CancelableDownload(canceler, httpClient.Do, req) if err != nil { - return nil, err + return nil, false, err } defer close(doneCh) if raw.StatusCode != http.StatusOK { - return nil, fmt.Errorf("Unable to fetch %q: %s", args.Server, raw.Status) + return nil, false, fmt.Errorf("Unable to fetch %q: %s", args.Server, raw.Status) } // Progress handler @@ -494,7 +494,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope // Create the target files f, err := os.Create(destName) if err != nil { - return nil, err + return nil, false, err } defer func() { _ = f.Close() }() @@ -506,19 +506,19 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope writer := internalIO.NewQuotaWriter(io.MultiWriter(f, sha256), args.Budget) size, err := io.Copy(writer, body) if err != nil { - return nil, err + return nil, false, err } // Validate hash result := fmt.Sprintf("%x", sha256.Sum(nil)) if result != fp { - return nil, fmt.Errorf("Hash mismatch for %q: %s != %s", args.Server, result, fp) + return nil, false, fmt.Errorf("Hash mismatch for %q: %s != %s", args.Server, result, fp) } // Parse the image imageMeta, imageType, err := getImageMetadata(destName) if err != nil { - return nil, err + return nil, false, err } info = &api.Image{} @@ -538,10 +538,10 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope err = f.Close() if err != nil { - return nil, err + return nil, false, err } } else { - return nil, fmt.Errorf("Unsupported protocol: %v", protocol) + return nil, false, fmt.Errorf("Unsupported protocol: %v", protocol) } // Override visiblity @@ -559,7 +559,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope return tx.CreateImage(ctx, args.ProjectName, info.Fingerprint, info.Filename, info.Size, info.Public, info.AutoUpdate, info.Architecture, info.CreatedAt, info.ExpiresAt, info.Properties, info.Type, nil) }) if err != nil { - return nil, fmt.Errorf("Failed creating image record: %w", err) + return nil, false, fmt.Errorf("Failed creating image record: %w", err) } // Image is in the DB now, don't wipe on-disk files on failure @@ -570,13 +570,13 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope if newDestName != destName { err = internalUtil.FileMove(destName, newDestName) if err != nil { - return nil, err + return nil, false, err } if util.PathExists(destName + ".rootfs") { err = internalUtil.FileMove(destName+".rootfs", newDestName+".rootfs") if err != nil { - return nil, err + return nil, false, err } } } @@ -592,7 +592,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope return tx.CreateImageSource(ctx, id, args.Server, protocol, args.Certificate, alias) }) if err != nil { - return nil, err + return nil, false, err } } @@ -600,7 +600,7 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope if args.StoragePool != "" { err = imageCreateInPool(s, info, args.StoragePool) if err != nil { - return nil, err + return nil, false, err } } @@ -610,11 +610,11 @@ func ImageDownload(ctx context.Context, r *http.Request, s *state.State, op *ope return tx.SetImageCachedAndLastUseDate(ctx, args.ProjectName, fp, time.Now().UTC()) }) if err != nil { - return nil, fmt.Errorf("Failed setting cached flag and last use date: %w", err) + return nil, false, fmt.Errorf("Failed setting cached flag and last use date: %w", err) } } logger.Info("Image downloaded", ctxMap) - return info, nil + return info, true, nil } diff --git a/cmd/incusd/images.go b/cmd/incusd/images.go index 81855532675..9a282fc35ef 100644 --- a/cmd/incusd/images.go +++ b/cmd/incusd/images.go @@ -428,7 +428,7 @@ func imgPostRemoteInfo(ctx context.Context, s *state.State, r *http.Request, req return nil, fmt.Errorf("must specify one of alias or fingerprint for init from image") } - info, err := ImageDownload(ctx, r, s, op, &ImageDownloadArgs{ + info, _, err := ImageDownload(ctx, r, s, op, &ImageDownloadArgs{ Server: req.Source.Server, Protocol: req.Source.Protocol, Certificate: req.Source.Certificate, @@ -546,7 +546,7 @@ func imgPostURLInfo(ctx context.Context, s *state.State, r *http.Request, req ap } // Import the image - info, err := ImageDownload(ctx, r, s, op, &ImageDownloadArgs{ + info, _, err := ImageDownload(ctx, r, s, op, &ImageDownloadArgs{ Server: url, Protocol: "direct", Alias: hash, @@ -2198,7 +2198,7 @@ func autoUpdateImage(ctx context.Context, s *state.State, op *operations.Operati default: } - newInfo, err = ImageDownload(ctx, nil, s, op, &ImageDownloadArgs{ + newInfo, _, err = ImageDownload(ctx, nil, s, op, &ImageDownloadArgs{ Server: source.Server, Protocol: source.Protocol, Certificate: source.Certificate, diff --git a/cmd/incusd/instances_post.go b/cmd/incusd/instances_post.go index c41bfdfd839..58227de1233 100644 --- a/cmd/incusd/instances_post.go +++ b/cmd/incusd/instances_post.go @@ -60,7 +60,7 @@ func ensureDownloadedImageFitWithinBudget(ctx context.Context, s *state.State, r return nil, err } - imgDownloaded, err := ImageDownload(ctx, r, s, op, &ImageDownloadArgs{ + imgDownloaded, created, err := ImageDownload(ctx, r, s, op, &ImageDownloadArgs{ Server: source.Server, Protocol: source.Protocol, Certificate: source.Certificate, @@ -78,13 +78,15 @@ func ensureDownloadedImageFitWithinBudget(ctx context.Context, s *state.State, r return nil, err } - // Add the image to the authorizer. - err = s.Authorizer.AddImage(s.ShutdownCtx, p.Name, imgDownloaded.Fingerprint) - if err != nil { - logger.Error("Failed to add image to authorizer", logger.Ctx{"fingerprint": imgDownloaded.Fingerprint, "project": p.Name, "error": err}) - } + if created { + // Add the image to the authorizer. + err = s.Authorizer.AddImage(s.ShutdownCtx, p.Name, imgDownloaded.Fingerprint) + if err != nil { + logger.Error("Failed to add image to authorizer", logger.Ctx{"fingerprint": imgDownloaded.Fingerprint, "project": p.Name, "error": err}) + } - s.Events.SendLifecycle(p.Name, lifecycle.ImageCreated.Event(imgDownloaded.Fingerprint, p.Name, op.Requestor(), logger.Ctx{"type": imgDownloaded.Type})) + s.Events.SendLifecycle(p.Name, lifecycle.ImageCreated.Event(imgDownloaded.Fingerprint, p.Name, op.Requestor(), logger.Ctx{"type": imgDownloaded.Type})) + } return imgDownloaded, nil } From c0643ad6946431580af7bf4c6318a76d53a477f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Graber?= Date: Wed, 25 Sep 2024 10:11:20 -0400 Subject: [PATCH 2/2] incusd/instances: Call placement scriptlet when target specified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1221, #1207 Signed-off-by: Stéphane Graber --- cmd/incusd/instance_post.go | 24 +++++++++++++++++++----- cmd/incusd/instances_post.go | 9 +++++++-- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/cmd/incusd/instance_post.go b/cmd/incusd/instance_post.go index 2932515bbb2..88024df6a32 100644 --- a/cmd/incusd/instance_post.go +++ b/cmd/incusd/instance_post.go @@ -315,8 +315,13 @@ func instancePost(d *Daemon, r *http.Request) response.Response { return response.SmartError(err) } - // If no specific server and a placement scriplet exists, call it with the candidates. - if targetMemberInfo == nil && s.GlobalConfig.InstancesPlacementScriptlet() != "" { + // Run instance placement scriptlet if enabled. + if s.GlobalConfig.InstancesPlacementScriptlet() != "" { + // If a target was specified, limit the list of candidates to that target. + if targetMemberInfo != nil { + targetCandidates = []db.NodeInfo{*targetMemberInfo} + } + leaderAddress, err := s.Cluster.LeaderAddress() if err != nil { return response.InternalError(err) @@ -335,9 +340,18 @@ func instancePost(d *Daemon, r *http.Request) response.Response { Reason: apiScriptlet.InstancePlacementReasonRelocation, } - targetMemberInfo, err = scriptlet.InstancePlacementRun(r.Context(), logger.Log, s, &req, targetCandidates, leaderAddress) - if err != nil { - return response.BadRequest(fmt.Errorf("Failed instance placement scriptlet: %w", err)) + if targetMemberInfo == nil { + // Get a new target. + targetMemberInfo, err = scriptlet.InstancePlacementRun(r.Context(), logger.Log, s, &req, targetCandidates, leaderAddress) + if err != nil { + return response.BadRequest(fmt.Errorf("Failed instance placement scriptlet: %w", err)) + } + } else { + // Validate the current target. + _, err = scriptlet.InstancePlacementRun(r.Context(), logger.Log, s, &req, targetCandidates, leaderAddress) + if err != nil { + return response.BadRequest(fmt.Errorf("Failed instance placement scriptlet: %w", err)) + } } } diff --git a/cmd/incusd/instances_post.go b/cmd/incusd/instances_post.go index 58227de1233..f6c7fde9aca 100644 --- a/cmd/incusd/instances_post.go +++ b/cmd/incusd/instances_post.go @@ -1095,8 +1095,13 @@ func instancesPost(d *Daemon, r *http.Request) response.Response { return response.BadRequest(err) } - if s.ServerClustered && !clusterNotification && targetMemberInfo == nil { - // Run instance placement scriptlet if enabled and no cluster member selected yet. + if s.ServerClustered && !clusterNotification { + // If a target was specified, limit the list of candidates to that target. + if targetMemberInfo != nil { + candidateMembers = []db.NodeInfo{*targetMemberInfo} + } + + // Run instance placement scriptlet if enabled. if s.GlobalConfig.InstancesPlacementScriptlet() != "" { leaderAddress, err := s.Cluster.LeaderAddress() if err != nil {