-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Add oadm migrate etcd-ttl
which encodes upstream TTL migration
#14559
Conversation
@ingvagabund taking the upstream and going to put it into openshift - we may need this in the future anyway |
b758e6c
to
86f0a54
Compare
Flake, wtf
Jun 9 20:26:32.845: INFO: At 2017-06-09 20:21:22 -0400 EDT - event
for nodeport-test-3z5g6: {kubelet nettest-node-2} MissingClusterDNS:
kubelet does not have ClusterDNS IP configured and cannot create Pod
using "ClusterFirst" policy. Falling back to DNSDefault policy.
|
[test] |
pkg/cmd/admin/migrate/etcd/ttl.go
Outdated
fmt.Fprintf(o.Out, "info: Attaching lease to %d entries\n", len(objectsResp.Kvs)) | ||
errors := 0 | ||
for _, kv := range objectsResp.Kvs { | ||
_, err := client.KV.Put(ctx, string(kv.Key), string(kv.Value), clientv3.WithLease(lease.ID)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make conditional so we don't stomp a changed value, gather items we need to retry?
return fmt.Errorf("unable to get objects to attach to the lease: %v", err) | ||
} | ||
|
||
lease, err := client.Lease.Grant(ctx, int64(o.leaseDuration/time.Second)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there any downsides to associating a lot of items with a single lease?
pkg/cmd/admin/migrate/etcd/ttl.go
Outdated
func (o *MigrateTTLReferenceOptions) Bind(flag *pflag.FlagSet) { | ||
flag.StringVar(&o.etcdAddress, "etcd-address", "", "Etcd address") | ||
flag.StringVar(&o.ttlKeysPrefix, "ttl-keys-prefix", "", "Prefix for TTL keys") | ||
flag.DurationVar(&o.leaseDuration, "lease-duration", time.Hour, "Lease duration (seconds granularity)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't set a default duration... I would require the caller to decide
// Make sure that ttlKeysPrefix is ended with "/" so that we only get children "directories". | ||
if !strings.HasSuffix(o.ttlKeysPrefix, "/") { | ||
o.ttlKeysPrefix += "/" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require duration to be > time.Second
All comments addressed, rebasing on master and then will merge
…On Tue, Jun 13, 2017 at 11:37 AM, OpenShift Bot ***@***.***> wrote:
Origin Action Required: Pull request cannot be automatically merged,
please rebase your branch from latest HEAD and push again
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#14559 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p2K6Q0uoYQYJuIavIpSXpErSqRM7ks5sDqy4gaJpZM4N1uF7>
.
|
86f0a54
to
085df3c
Compare
[merge] |
continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 19 |
Evaluated for origin merge up to 085df3c |
[severity:blocker] |
This is in support of v2 -> v3
085df3c
to
bfec1fc
Compare
[test] |
Evaluated for origin test up to bfec1fc |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2252/) (Base Commit: 811a267) |
@ingvagabund this should be in the next 3.6 build so you can switch to using this |
Will check repos and update. @smarterclayton @sdodson thanks |
Should be in 3.6.112
…On Fri, Jun 16, 2017 at 3:48 AM, Jan Chaloupka ***@***.***> wrote:
Will check repos and update. @smarterclayton
<https://github.com/smarterclayton> @sdodson <https://github.com/sdodson>
thanks
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14559 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAC8IQhdGn8sToKYriIX26_L4kciVIKUks5sEjM_gaJpZM4N1uF7>
.
|
This is in support of v2 -> v3
[test] @sdodson