Make the healthcheck flags compatible with Docker CLI #3508

csomh · 2019-07-08T09:16:23Z

Docker CLI calls the healthcheck flags --health-*, instead of
--healthcheck-*. Introduce the former, in order to keep compatibility,
and alias the later, in order to avoid breaking current usage.

See discussion on #3455.

openshift-ci-robot · 2019-07-08T09:16:43Z

Hi @csomh. Thanks for your PR.

I'm waiting for a containers or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mheon · 2019-07-08T13:45:41Z

cmd/podman/common.go

+	createFlags.String(
+		"health-cmd", "",
+		"set a healthcheck command for the container ('none' disables the existing healthcheck)",
+	)
 	createFlags.String(
 		"healthcheck-command", "",


I think Cobra allows aliases for flags - it'd be better to add these as aliases to the existing flags, not entirely new ones

Can you point me to some docs, please? I've looked but could not find anything. This is why I've followed the same pattern that was used for the --net, --network pair.

(Cobra allows shorthands, as in --publish, -p with StringP, but that's not what is needed in this case.)

I'm almost certain we used this somewhere... Let me poke around

I might have been thinking of urfave/cli instead, which AFAIK allowed arbitrary numbers of flag aliases. Which, in cases like this, was a good bit more convenient...

mheon · 2019-07-08T14:09:23Z

/ok-to-test
/approve

openshift-ci-robot · 2019-07-08T14:09:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csomh, mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mheon · 2019-07-08T14:10:26Z

cmd/podman/shared/create.go

@@ -91,7 +91,11 @@ func CreateContainer(ctx context.Context, c *GenericCLIResults, runtime *libpod.

 		var healthCheckCommandInput string
 		// if the user disabled the healthcheck with "none", we skip adding it
-		healthCheckCommandInput = c.String("healthcheck-command")
+		healthCheckCommandInput = c.String("health-cmd")


I'd prefer we error here if both are set, instead of unconditionally overwriting

rh-atomic-bot · 2019-07-08T17:42:08Z

☔ The latest upstream changes (presumably #3517) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2019-07-08T18:03:44Z

Inside of cobra they explain how to do this.

Example #2: You want to alias two flags. aka --old-flag-name == --new-flag-name

func aliasNormalizeFunc(f *pflag.FlagSet, name string) pflag.NormalizedName {
	switch name {
	case "old-flag-name":
		name = "new-flag-name"
		break
	}
	return pflag.NormalizedName(name)
}

myFlagSet.SetNormalizeFunc(aliasNormalizeFunc)

csomh · 2019-07-09T10:20:08Z

Thanks @rhatdan! That's what I was looking for.

I've updated the PR to use aliases instead.

Varlink and GenericCLIResults were not changed, but I'm not entirely confident, that I understand those parts correctly, so let me know if those also need a change.

docs/podman-create.1.md

docs/podman-run.1.md

mheon · 2019-07-09T13:40:15Z

Slight touchups needed on the manpages, otherwise LGTM

mheon · 2019-07-10T14:35:56Z

@csomh Mind rebasing? There are fixes for test failures on master

csomh · 2019-07-10T16:05:14Z

@mheon some are still failing. Would it make sense to rebase onto the latest green commit instead?

mheon · 2019-07-10T16:36:12Z

Checking, let me see if we're looking at flakes or real failures

mheon · 2019-07-10T16:37:12Z

[+2083s] panic: unable to find generic flag for varlink health-cmd
[+2083s] 
[+2083s] goroutine 1 [running]:
[+2083s] github.com/containers/libpod/cmd/podman/shared.GenericCLIResults.Find(...)
[+2083s] 	cmd/podman/shared/intermediate_varlink.go:433
[+2083s] github.com/containers/libpod/cmd/podman/shared.GenericCLIResults.MakeVarlink(0xc000390330, 0xc0003fd770, 0x2, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
[+2083s] 	cmd/podman/shared/intermediate_varlink.go:107 +0x64b1
[+2083s] github.com/containers/libpod/pkg/adapter.(*LocalRuntime).Run(0xc0000ae048, 0x1a709a0, 0xc0000aa018, 0x2a07080, 0x7d, 0x0, 0x0, 0x0)
[+2083s] 	pkg/adapter/containers_remote.go:463 +0x77
[+2083s] main.runCmd(0x2a07080, 0x0, 0x0)
[+2083s] 	cmd/podman/run.go:60 +0x164
[+2083s] main.glob..func29(0x29a4180, 0xc0003fd770, 0x2, 0x3, 0x0, 0x0)
[+2083s] 	cmd/podman/run.go:23 +0x86
[+2083s] github.com/containers/libpod/vendor/github.com/spf13/cobra.(*Command).execute(0x29a4180, 0xc0000b4020, 0x3, 0x3, 0x29a4180, 0xc0000b4020)
[+2083s] 	vendor/github.com/spf13/cobra/command.go:826 +0x465
[+2083s] github.com/containers/libpod/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0x29a9900, 0xc00009ccf0, 0x7fffb074ded9, 0x3e)
[+2083s] 	vendor/github.com/spf13/cobra/command.go:914 +0x2fc
[+2083s] github.com/containers/libpod/vendor/github.com/spf13/cobra.(*Command).Execute(...)
[+2083s] 	vendor/github.com/spf13/cobra/command.go:864
[+2083s] main.main()
[+2083s] 	cmd/podman/main.go:147 +0x88

mheon · 2019-07-10T16:37:22Z

I think we might have a real panic here

csomh · 2019-07-10T16:51:48Z

Of course we had... un-did the changes in MakeVarlink().

mheon · 2019-07-10T17:57:48Z

Last failure looks like a flake, restarted

rhatdan · 2019-07-12T20:06:51Z

Command completions in contrib/bash/podman also need to be updated.

csomh · 2019-07-13T08:10:18Z

Do you mean completions/bash/podman?

--health-start-period was added to that one (the other --health-* flags were already there), and --no-healthcheck removed, as it's currently not an option in podman (--health-cmd=none might mean the same 😕)

I did not update completion with the --healhtcheck-* variants, as I think this would make completing --health confusing (would bring up both variants, users might be confused which one to use), and might lead to users crafting incompatible commands as a result.

Let me know if you think otherwise.

rhatdan · 2019-07-12T20:06:14Z

docs/podman-create.1.md

@@ -268,26 +268,26 @@ The following example maps uids 0-2000 in the container to the uids 30000-31999

 Add additional groups to run as

-**--healthcheck-command**=*command*
+**--health-cmd**, **--healthcheck-command**=*command*


Can you just remove the old options, they should be gone now from --help, so no reason to document, as long as people using the old option continue to work, we don't need to document.

rhatdan · 2019-07-13T09:58:40Z

Good on the completions. I agree on not exposing --healthcheck* at all.

Bottom line is remove it from being exposed anywhere but support it if a user used it within a script.

csomh · 2019-07-14T20:23:36Z

Removed --healthcheck-* from the docs and updated all the tests to use the documented flags.

csomh · 2019-07-15T19:42:35Z

/retest

rh-atomic-bot · 2019-07-15T19:59:51Z

☔ The latest upstream changes (presumably #3574) made this pull request unmergeable. Please resolve the merge conflicts.

Signed-off-by: Hunor Csomortáni <[email protected]>

Docker CLI calls the healthcheck flags "--health-*", instead of "--healthcheck-*". Introduce the former, in order to keep compatibility, and alias the later, in order to avoid breaking current usage. Change "--healthcheck-*" to "--health-*" in the docs and tests. Signed-off-by: Hunor Csomortáni <[email protected]>

Signed-off-by: Hunor Csomortáni <[email protected]>

csomh · 2019-07-16T06:37:25Z

/retest

csomh · 2019-07-16T06:45:07Z

@rhatdan @mheon could you please help me to re-run the failing tests? They don't seem to be related to the changes made. Prow does not seem to listen to my commands. Thanks!

mheon · 2019-07-16T13:40:50Z

Restarted them both - looked like known flakes

rhatdan · 2019-07-16T14:18:35Z

LGTM
@baude @mheon @giuseppe @vrothberg @ashley-cui @QiWang19 PTAL

TomSweeneyRedHat · 2019-07-16T18:26:16Z

LGTM and happy green test buttons. TYVM @csomh !

mheon · 2019-07-16T19:05:20Z

Merging
/lgtm

openshift-ci-robot requested review from baude and rhatdan July 8, 2019 09:16

openshift-ci-robot added the size/L label Jul 8, 2019

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 8, 2019

mheon reviewed Jul 8, 2019

View reviewed changes

openshift-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 8, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2019

mheon reviewed Jul 8, 2019

View reviewed changes

openshift-ci-robot added size/M and removed size/L labels Jul 9, 2019

mheon added the Release Notes 1.5.0 label Jul 9, 2019

mheon reviewed Jul 9, 2019

View reviewed changes

docs/podman-create.1.md Outdated Show resolved Hide resolved

mheon reviewed Jul 9, 2019

View reviewed changes

docs/podman-run.1.md Outdated Show resolved Hide resolved

stefanb2 mentioned this pull request Jul 12, 2019

podman incompatiblity with healthcheck from docker image #3507

Closed

rhatdan reviewed Jul 13, 2019

View reviewed changes

Hunor Csomortáni added 3 commits July 16, 2019 05:50

Completion: --no-healthcheck is not an option

51c00ea

Signed-off-by: Hunor Csomortáni <[email protected]>

Completion: complete "--health-start-period" in bash

47c1fe6

Signed-off-by: Hunor Csomortáni <[email protected]>

openshift-ci-robot assigned mheon Jul 16, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2019

openshift-merge-robot merged commit 400851a into containers:master Jul 16, 2019

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the healthcheck flags compatible with Docker CLI #3508

Make the healthcheck flags compatible with Docker CLI #3508

csomh commented Jul 8, 2019

openshift-ci-robot commented Jul 8, 2019

mheon Jul 8, 2019

csomh Jul 8, 2019

mheon Jul 8, 2019

mheon Jul 8, 2019

mheon commented Jul 8, 2019

openshift-ci-robot commented Jul 8, 2019

mheon Jul 8, 2019

rh-atomic-bot commented Jul 8, 2019

rhatdan commented Jul 8, 2019

csomh commented Jul 9, 2019

mheon commented Jul 9, 2019

mheon commented Jul 10, 2019

csomh commented Jul 10, 2019

mheon commented Jul 10, 2019

mheon commented Jul 10, 2019

mheon commented Jul 10, 2019

csomh commented Jul 10, 2019

mheon commented Jul 10, 2019

rhatdan commented Jul 12, 2019

csomh commented Jul 13, 2019

rhatdan Jul 12, 2019

rhatdan commented Jul 13, 2019

csomh commented Jul 14, 2019

csomh commented Jul 15, 2019

rh-atomic-bot commented Jul 15, 2019

csomh commented Jul 16, 2019

csomh commented Jul 16, 2019

mheon commented Jul 16, 2019

rhatdan commented Jul 16, 2019

TomSweeneyRedHat commented Jul 16, 2019

mheon commented Jul 16, 2019

Make the healthcheck flags compatible with Docker CLI #3508

Make the healthcheck flags compatible with Docker CLI #3508

Conversation

csomh commented Jul 8, 2019

openshift-ci-robot commented Jul 8, 2019

mheon Jul 8, 2019

Choose a reason for hiding this comment

csomh Jul 8, 2019

Choose a reason for hiding this comment

mheon Jul 8, 2019

Choose a reason for hiding this comment

mheon Jul 8, 2019

Choose a reason for hiding this comment

mheon commented Jul 8, 2019

openshift-ci-robot commented Jul 8, 2019

mheon Jul 8, 2019

Choose a reason for hiding this comment

rh-atomic-bot commented Jul 8, 2019

rhatdan commented Jul 8, 2019

csomh commented Jul 9, 2019

mheon commented Jul 9, 2019

mheon commented Jul 10, 2019

csomh commented Jul 10, 2019

mheon commented Jul 10, 2019

mheon commented Jul 10, 2019

mheon commented Jul 10, 2019

csomh commented Jul 10, 2019

mheon commented Jul 10, 2019

rhatdan commented Jul 12, 2019

csomh commented Jul 13, 2019

rhatdan Jul 12, 2019

Choose a reason for hiding this comment

rhatdan commented Jul 13, 2019

csomh commented Jul 14, 2019

csomh commented Jul 15, 2019

rh-atomic-bot commented Jul 15, 2019

csomh commented Jul 16, 2019

csomh commented Jul 16, 2019

mheon commented Jul 16, 2019

rhatdan commented Jul 16, 2019

TomSweeneyRedHat commented Jul 16, 2019

mheon commented Jul 16, 2019