support ipv6 #1769
support ipv6 #1769
Conversation
|
jenkins Failed because All nodes of label ‘gce_1.9.x’ are offline |
|
Does Jenkins CI work ? |
There was a problem hiding this comment.
Thanks for your continued effort @zmlcc. I actually do think there is still upgrade/migration impact. In the process described in the upgrade user guide, the ceph components only get their container version updated, e.g. with something like kubectl -n rook set image replicaset/rook-ceph-mon0 rook-ceph-mon=rook/ceph:master. This means that there will be pods still using the legacy commands and flags. You can see that the legacy ceph commands (commands that are NOT under the new ceph subcommand) are still supported by being directly added to the main rook command here: https://github.com/rook/rook/blob/master/cmd/rook/main.go#L40
I think we still need migration support with this flag name change. Perhaps just keep both the old and new name flags and probably an error check to make sure that both are not specified. What do you think?
Jenkinsfile
Outdated
| @@ -204,3 +204,4 @@ def notifySlack(String buildStatus) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
i think this extra blank line can be removed now
|
oh and yes, Jenkins CI should be working again now, we will see how the test run goes after we resolve the migration/upgrade support issue. |
|
@jbw976 Thanks for your explanation |
|
@jbw976 Any comment？ |
cmd/rook/ceph/mds.go
Outdated
| if err := flags.VerifyRequiredFlags(mdsCmd, required); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| renamed := [][2]string{{"public-ip", "public-ipv4"}, {"private-ip", "private-ipv4"}} |
There was a problem hiding this comment.
could the set of renamed flags be defined in just one place? (perhaps the ceph root command)
There was a problem hiding this comment.
define verifyRenamedFlags function in ceph.go and call it in each ceph subcommand
cmd/rook/ceph/mgr.go
Outdated
| if err := flags.VerifyRequiredFlags(mgrCmd, required); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| renamed := [][2]string{{"public-ip", "public-ipv4"}, {"private-ip", "private-ipv4"}} | ||
| if err := flags.VerifyRenamedFlags(mdsCmd, renamed); err != nil { |
There was a problem hiding this comment.
the mdsCmd is accidentally being checked here when you want to use the command that this file is for. this issue exists for all the other commands.
There was a problem hiding this comment.
Thanks for your reminder， I corrected all the mistakes
pkg/util/flags/flags.go
Outdated
| @@ -37,6 +37,22 @@ func VerifyRequiredFlags(cmd *cobra.Command, requiredFlags []string) error { | |||
| return createRequiredFlagError(cmd.Name(), missingFlags) | |||
| } | |||
|
|
|||
| func VerifyRenamedFlags(cmd *cobra.Command, renamedFlags [][2]string) error { | |||
There was a problem hiding this comment.
this function might be easier to read/maintain (especially where it is called and where it is tested) if it used a struct instead of a 2 element slice of strings. For example renamedFlags []RenamedFlag, where the type RenamedFlag is defined as:
type RenamedFlag struct {
NewFlagName string
OldFlagName string
}
There was a problem hiding this comment.
Refactored under your suggestion
|
@jbw976 I update the codes under your suggestion. Please review. |
deprecate NetworkInfo.PublicAddrIPv4, use NetworkInfo.PublicAddr instead deprecate NetworkInfo.ClusterAddrIPv4, use NetworkInfo.ClusterAddr instead deprecate command flag public-ipv4, use public-ip instead deprecate command flag private-ipv4, use private-ip instead change ROOK_PUBLIC_IPV4 to ROOK_PUBLIC_IP change ROOK_PRIVATE_IPV4 to ROOK_PRIVATE_IP Signed-off-by: Zhang Miaolei <zmlcc@outlook.com>
|
@jbw976 Any comment？ I notice jenkins check changed from success to failure due to expired |
Close #1630 by accident， reopen here
@jbw976 I notice there is a significant change in master branch:
all ceph related components were called in the
cephsubcommandSo the backwards incompatibility of flag and env vars is considered acceptable.
After upgrading there should be no pods that are using the old command and old flags and env vars.
Description of your changes:
join host and port using
net.JoinHostPortrename NetworkInfo.PublicAddrIPv4 to NetworkInfo.PublicAddr
rename NetworkInfo.ClusterAddrIPv4 to NetworkInfo.ClusterAddr
rename command flag public-ipv4 to public-ip
rename command flag private-ipv4 to private-ip
rename ROOK_PUBLIC_IPV4 to ROOK_PUBLIC_IP
rename ROOK_PRIVATE_IPV4 to ROOK_PRIVATE_IP
Which issue is resolved by this Pull Request:
Resolves #1629