chore: Validate artifact drivers on server start. re #10219 by JPZ13 · Pull Request #36 · pipekit/argo-workflows

JPZ13 · 2025-07-29T13:20:14Z

Fixes argoproj#10219

Please do not open a pull request until you have checked ALL of these:

Create the PR as draft .
Run make pre-commit -B to fix codegen and lint problems.
Sign-off your commits (otherwise the DCO check will fail).
Use a conventional commit message (otherwise the commit message check will fail).
"Fixes #" is in both the PR title (for release notes) and this description (to automatically link and close the issue).
Add unit or e2e tests. Say how you tested your changes. If you changed the UI, attach screenshots.
Github checks are green.
Once required tests have passed, mark your PR "Ready for review".

If changes were requested, and you've made them, dismiss the review to get it reviewed again.

server/apiserver/argoserver.go

JPZ13 · 2025-07-29T16:10:08Z

@Joibel - let me know when the base artifact-plugins branch is rebased with the logging changes. I'll update this PR once I get your go-ahead

Joibel · 2025-07-30T08:54:54Z

Rebase is done

Copilot

Pull Request Overview

This PR adds validation for artifact drivers during server startup to ensure they are properly configured and accessible. The validation checks that artifact driver images are present in the server pod and that connections to the drivers can be established.

Adds image validation to verify artifact driver images are included in the server deployment
Adds connection validation to test artifact driver accessibility during startup
Includes a new utility function to get the hostname from environment variables

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

File	Description
util/env/env.go	Adds GetHostname utility function to retrieve HOSTNAME environment variable
util/env/env_test.go	Updates test assertions from assert to require and adds tests for GetHostname function
server/apiserver/argoserver.go	Implements artifact driver validation logic with image and connection checks
server/apiserver/argoserver_test.go	Adds comprehensive test coverage for artifact driver image validation

server/apiserver/argoserver.go

Copilot · 2025-07-31T13:00:34Z

server/apiserver/argoserver.go

+	}
+
+	if len(images) > 0 {
+		errorMsg := fmt.Sprintf("Artifact driver validation failed: The following artifact driver images are not present in the server pod: %v. Please ensure all artifact driver images are included in the argo-server pod.", images)


The error message mentions 'server pod' but should be more specific about checking the deployment, since the validation logic in the test suggests it should check the deployment template rather than just the current pod.

Suggested change

errorMsg := fmt.Sprintf("Artifact driver validation failed: The following artifact driver images are not present in the server pod: %v. Please ensure all artifact driver images are included in the argo-server pod.", images)

errorMsg := fmt.Sprintf("Artifact driver validation failed: The following artifact driver images are not present in the current pod: %v. Please ensure all artifact driver images are included in the deployment template for the argo-server.", images)

server/apiserver/argoserver_test.go

util/env/env_test.go

server/apiserver/argoserver.go

server/apiserver/argoserver_test.go

Joibel · 2025-07-31T13:16:12Z

server/apiserver/argoserver.go

+			defer wg.Done()
+
+			// Create a new driver connection
+			pluginDriver, err := plugin.NewDriver(ctx, driver.Name, driver.Name.SocketPath(), 5) // 5 second timeout


This '5' is a configuration element from the yaml: driver.ConnectionTimeoutSeconds so use that please

Do you want me to add it as a field on the ArtifactDriver type? I don't see it currently or anywhere else on the configmap? https://github.com/pipekit/argo-workflows/blob/artifact-plugins/config/config.go#L127

Ah, yes. We should move it to where you've suggested from

argo-workflows/pkg/apis/workflow/v1alpha1/workflow_types.go

Line 3143 in 3e9e872

ConnectionTimeoutSeconds int32 `json:"connectionTimeoutSeconds,omitempty" protobuf:"varint,3,opt,name=connectionTimeoutSeconds"`

I think.
Does that make more sense, I think it does.

It would let you specify a timeout for each grpc server, which might be useful but also might be lots of config. Maybe default to 5 if not set?

server/apiserver/argoserver.go

Joibel

I feel like this is missing a change to workflow_types.go or something - generated.pb.go has changed but that's a generated file. The file from which it is generated should be included too.

JPZ13 · 2025-09-04T11:03:21Z

I feel like this is missing a change to workflow_types.go or something - generated.pb.go has changed but that's a generated file. The file from which it is generated should be included too.

Missing a change with respect to the default branch that I should rebase on top of or missing a change that I should implement?

JPZ13 · 2025-09-04T11:41:16Z

@Joibel - I think I've updated. Can you confirm?

JPZ13 · 2025-09-04T12:07:03Z

Some of these changes are some strays that got brought in from the master branch. If we rebase artifact-plugins on top of master, it'll fix the non-related additions

Signed-off-by: Alan Clucas <alan@clucas.org>

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com> test connections Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com> refactor get images Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

util/env/env_test.go

JPZ13 · 2025-09-08T18:29:22Z

@Joibel if it's a one line change, feel free to just push a commit, approve, and merge for the sake of time

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

JPZ13 commented Jul 29, 2025

View reviewed changes

server/apiserver/argoserver.go Outdated Show resolved Hide resolved

Joibel requested changes Jul 29, 2025

View reviewed changes

Joibel force-pushed the artifact-plugins branch from 8832395 to 9351288 Compare July 29, 2025 17:05

JPZ13 force-pushed the 10219-ensure-grpc-servers-present-on-argo-server-startup branch from b7a017a to 772114c Compare July 30, 2025 13:55

JPZ13 requested review from Joibel July 30, 2025 15:51

JPZ13 marked this pull request as ready for review July 31, 2025 12:46

Joibel requested a review from Copilot July 31, 2025 12:59

Copilot AI reviewed Jul 31, 2025

View reviewed changes

Joibel requested changes Jul 31, 2025

View reviewed changes

Joibel requested changes Aug 5, 2025

View reviewed changes

JPZ13 force-pushed the 10219-ensure-grpc-servers-present-on-argo-server-startup branch 3 times, most recently from bf04d54 to a8217b1 Compare September 4, 2025 11:40

JPZ13 requested a review from Joibel September 4, 2025 11:41

JPZ13 force-pushed the artifact-plugins branch from 9e6b415 to 0344a8f Compare September 8, 2025 11:42

JPZ13 force-pushed the 10219-ensure-grpc-servers-present-on-argo-server-startup branch from a8217b1 to 0a1a897 Compare September 8, 2025 11:43

Joibel and others added 8 commits September 8, 2025 13:55

feat: initial commit for artifact plugins

b0f2bb3

Signed-off-by: Alan Clucas <alan@clucas.org>

chore: Validate artifact drivers on server start. re: argoproj#10219

c98ec98

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com> test connections Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com> refactor get images Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

fix server lint errors

05b8d73

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

use require instead of assert

eb760e6

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

remove generation

c9ffd7d

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

use t.setenv

f6f938b

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

remove deployment in server test

10a5631

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

structure logs

de9a542

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

JPZ13 added 4 commits September 8, 2025 13:55

fix error overlap

a9da605

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

run codegen

7cccf0b

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

fix comment

8d57ccd

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

run codegen

0387c96

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

JPZ13 force-pushed the 10219-ensure-grpc-servers-present-on-argo-server-startup branch from 0a1a897 to 0387c96 Compare September 8, 2025 11:56

JPZ13 added 2 commits September 8, 2025 14:28

refactor to use util fn for get pod name

fbd4458

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

update tests

3d00749

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

Joibel requested changes Sep 8, 2025

View reviewed changes

util/env/env_test.go Outdated Show resolved Hide resolved

switch back to assert

dec565d

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

JPZ13 requested a review from Joibel September 8, 2025 18:46

Joibel approved these changes Sep 9, 2025

View reviewed changes

JPZ13 merged commit 314fab8 into artifact-plugins Sep 9, 2025
3 checks passed

JPZ13 deleted the 10219-ensure-grpc-servers-present-on-argo-server-startup branch September 9, 2025 09:17

	errorMsg := fmt.Sprintf("Artifact driver validation failed: The following artifact driver images are not present in the server pod: %v. Please ensure all artifact driver images are included in the argo-server pod.", images)
	errorMsg := fmt.Sprintf("Artifact driver validation failed: The following artifact driver images are not present in the current pod: %v. Please ensure all artifact driver images are included in the deployment template for the argo-server.", images)

Conversation

JPZ13 commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JPZ13 commented Jul 29, 2025

Uh oh!

Joibel commented Jul 30, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Joibel Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

JPZ13 Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Joibel Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

JPZ13 Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Joibel left a comment

Choose a reason for hiding this comment

Uh oh!

JPZ13 commented Sep 4, 2025

Uh oh!

JPZ13 commented Sep 4, 2025

Uh oh!

JPZ13 commented Sep 4, 2025

Uh oh!

Uh oh!

JPZ13 commented Sep 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JPZ13 commented Jul 29, 2025 •

edited

Loading