Skip to content
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.

Sign up for GitHub

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

Enables the use of offline reason for all "offline" statuses (those that are not ONLINE/REPORTED) and makes it optional #7042

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Enables the use of offline reason for all "offline" statuses (those that are not ONLINE/REPORTED) and makes it optional #7042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a19ace
offline is anything not online/reported
mitchell852 Aug 29, 2022
25e5520
makes offline reason optional
mitchell852 Aug 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions traffic_ops/traffic_ops_golang/server/put_status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,12 +121,10 @@ func UpdateStatusHandler(w http.ResponseWriter, r *http.Request) {
return
}

if *status.Name == tc.CacheStatusAdminDown.String() || *status.Name == tc.CacheStatusOffline.String() {
if reqObj.OfflineReason == nil {
api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("offlineReason is required for "+tc.CacheStatusAdminDown.String()+" or "+tc.CacheStatusOffline.String()+" status"), nil)
return
if *status.Name != tc.CacheStatusOnline.String() && *status.Name != tc.CacheStatusReported.String() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I make a server have the status ONLINE1 or ONLINE_BUT_NOT_IN_SERVICE I then have to enter a reason why my ostensibly online server is offline?

Copy link
Member Author

@mitchell852 mitchell852 Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, unless we made "offline reason" always optional (never required as it is now) which makes more sense to me anyhow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ONLINE1 or ONLINE_BUT_NOT_IN_SERVICE is not considered "online" from TC's perspective :)

Copy link
Member Author

@mitchell852 mitchell852 Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well i guess it could be for some servers :) i.e. TP or TO where status has no impact and is purely informational

Copy link
Contributor

@ocket8888 ocket8888 Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Server statuses should not be mutable. It's one of a million places where we hijacked a concept to provide arbitrary informative tagging, which is better provided by actual tags. As it is, there's no guarantee that a status with any given name exists, or that a status with any given ID won't be renamed arbitrarily at any moment. Doing these kinds of checks becomes a lot easier once we can say what the actual values are and should mean.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, i get it but that ship has sailed (the immutable part), we already have many "offline" status types in production (i.e. TEST, PRE_PROD, DECOM, etc) that are all "offline" yet you can't add an offline reason to them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can be retconned:

  1. Implement tags
  2. Migrate existing, unrecognized statuses to tags
  3. Apply those tags on the servers that use those statuses
  4. Convert all unknown statuses to "OFFLINE"
  5. Eliminate the concept of a "Status" as an ATC object, use constant strings only.

if reqObj.OfflineReason != nil {
*reqObj.OfflineReason = inf.User.UserName + ": " + *reqObj.OfflineReason
}
*reqObj.OfflineReason = inf.User.UserName + ": " + *reqObj.OfflineReason
} else {
reqObj.OfflineReason = nil
}
Expand Down
2 changes: 1 addition & 1 deletion traffic_portal/app/src/common/modules/dialog/select/status/DialogSelectStatusController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ var DialogSelectStatusController = function(server, statuses, $scope, $uibModalI

$scope.offline = function () {
var selectedStatus = _.find(statuses, function(status){ return parseInt(status.id) == parseInt($scope.selectedStatusId) });
return selectedStatus && (selectedStatus.name == "ADMIN_DOWN" || selectedStatus.name == "OFFLINE");
return selectedStatus && (selectedStatus.name != "ONLINE" && selectedStatus.name != "REPORTED");
};

};
Expand Down
4 changes: 2 additions & 2 deletions traffic_portal/app/src/common/modules/dialog/select/status/dialog.select.status.tpl.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ <h5 ng-if="needsUpdates(server)">Note: Config file updates will be queued on ALL
<div class="row" ng-show="offline()">
<div class="col-sm-12 col-md-12">
<div class="form-group" ng-class="{'has-error': hasError(statusForm.offlineReason), 'has-feedback': hasError(statusForm.offlineReason)}">
<label class="control-label" for="offlineReason">Offline Reason *
<label class="control-label" for="offlineReason">Offline Reason
<small class="input-error" ng-show="hasPropertyError(statusForm.offlineReason, 'maxlength')">Too Long</small>
</label>
<input id="offlineReason" name="offlineReason" type="text" class="form-control" ng-model="status.offlineReason" ng-maxlength="256" ng-required="offline()" autofocus>
<input id="offlineReason" name="offlineReason" type="text" class="form-control" ng-model="status.offlineReason" ng-maxlength="256" autofocus>
<span ng-show="hasError(statusForm.offlineReason)" class="form-control-feedback"><i class="fa fa-times"></i></span>
</div>
</div>
Expand Down