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
rgw: update period if period does not exist #8828
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -151,7 +151,7 @@ func removeObjectStoreFromMultisite(objContext *Context, spec cephv1.ObjectStore | |
if err != nil { | ||
return errors.Wrap(err, "failed to update period after removing an endpoint from the zone") | ||
} | ||
logger.Infof("successfully updated period for realm %v after removal of object-store %v", objContext.Realm, objContext.Name) | ||
logger.Infof("successfully updated period for realm %q after removal of object-store %q", objContext.Realm, objContext.Name) | ||
|
||
return nil | ||
} | ||
|
@@ -373,9 +373,9 @@ func createMultisite(objContext *Context, endpointArg string) error { | |
if err != nil { | ||
return errorOrIsNotFound(err, "failed to create ceph realm %q, for reason %q", objContext.ZoneGroup, output) | ||
} | ||
logger.Debugf("created realm %v", objContext.Realm) | ||
logger.Debugf("created realm %q", objContext.Realm) | ||
} else { | ||
return errorOrIsNotFound(err, "radosgw-admin realm get failed with code %d, for reason %q. %v", strconv.Itoa(code), output, string(kerrors.ReasonForError(err))) | ||
return errorOrIsNotFound(err, "'radosgw-admin realm get' failed with code %d, for reason %q. %v", strconv.Itoa(code), output, string(kerrors.ReasonForError(err))) | ||
} | ||
} | ||
|
||
|
@@ -389,9 +389,9 @@ func createMultisite(objContext *Context, endpointArg string) error { | |
if err != nil { | ||
return errorOrIsNotFound(err, "failed to create ceph zone group %q, for reason %q", objContext.ZoneGroup, output) | ||
} | ||
logger.Debugf("created zone group %v", objContext.ZoneGroup) | ||
logger.Debugf("created zone group %q", objContext.ZoneGroup) | ||
} else { | ||
return errorOrIsNotFound(err, "radosgw-admin zonegroup get failed with code %d, for reason %q", strconv.Itoa(code), output) | ||
return errorOrIsNotFound(err, "'radosgw-admin zonegroup get' failed with code %d, for reason %q", strconv.Itoa(code), output) | ||
} | ||
} | ||
|
||
|
@@ -405,19 +405,32 @@ func createMultisite(objContext *Context, endpointArg string) error { | |
if err != nil { | ||
return errorOrIsNotFound(err, "failed to create ceph zone %q, for reason %q", objContext.Zone, output) | ||
} | ||
logger.Debugf("created zone %v", objContext.Zone) | ||
logger.Debugf("created zone %q", objContext.Zone) | ||
} else { | ||
return errorOrIsNotFound(err, "radosgw-admin zone get failed with code %d, for reason %q", strconv.Itoa(code), output) | ||
return errorOrIsNotFound(err, "'radosgw-admin zone get' failed with code %d, for reason %q", strconv.Itoa(code), output) | ||
} | ||
} | ||
|
||
// check if the period exists | ||
output, err = runAdminCommand(objContext, false, "period", "get") | ||
if err != nil { | ||
code, err := exec.ExtractExitCode(err) | ||
// ENOENT means “No such file or directory” | ||
if err == nil && code == int(syscall.ENOENT) { | ||
// period does not exist and so needs to be created | ||
updatePeriod = true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the period doesn't exist, won't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... that's a good question. I assume There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I verified that if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIR in the https://bugzilla.redhat.com/show_bug.cgi?id=2002220 setup, the
So IMO it is better to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This implementation avoids using |
||
} else { | ||
return errorOrIsNotFound(err, "'radosgw-admin period get' failed with code %d, for reason %q", strconv.Itoa(code), output) | ||
} | ||
} | ||
|
||
if updatePeriod { | ||
// the period will help notify other zones of changes if there are multi-zones | ||
_, err := runAdminCommand(objContext, false, "period", "update", "--commit") | ||
_, err = runAdminCommand(objContext, false, "period", "update", "--commit") | ||
if err != nil { | ||
return errorOrIsNotFound(err, "failed to update period") | ||
} | ||
logger.Debugf("updated period for realm %v", objContext.Realm) | ||
logger.Debugf("updated period for realm %q", objContext.Realm) | ||
} | ||
|
||
logger.Infof("Multisite for object-store: realm=%s, zonegroup=%s, zone=%s", objContext.Realm, objContext.ZoneGroup, objContext.Zone) | ||
|
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.
Can we have info or debug log here?
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.
It isn't really needed since the command for the create below is debug logged
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.
More logging is always better at least as an indication for humans. Anyway, I'm not gonna hold for this
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'll add it in a follow-up