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

No operator action required issues #1040

Merged
merged 8 commits into from
Jun 25, 2021
Merged

No operator action required issues #1040

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
71a8681
Introduced new error code: ERR_INFRA_REQUEST_THROTTLING
grolu Jun 22, 2021
c885ac6
small text corrections
grolu Jun 22, 2021
c2d1ee4
Rm dedicated icon for temporary errors
grolu Jun 23, 2021
33e1b22
Update frontend/src/views/ShootList.vue
grolu Jun 23, 2021
21d2734
Apply suggestions from code review
grolu Jun 24, 2021
bfd7aa4
changes as discussed
grolu Jun 24, 2021
1b435c1
removed icon function
grolu Jun 24, 2021
3697542
Added ERR_RETRYABLE_INFRA_DEPENDENCIES to known errors
grolu Jun 25, 2021
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
15 changes: 13 additions & 2 deletions frontend/src/components/ShootMessageDetails.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,9 @@ SPDX-License-Identifier: Apache-2.0
<div v-for="(lastErrorDescription, index) in errorDescriptions" :key="index">
<v-divider v-if="index > 0" class="my-2"></v-divider>
<v-alert
v-for="({ description, userError, infraAccountError }) in lastErrorDescription.errorCodeObjects" :key="description"
v-for="({ description, userError, temporaryError, infraAccountError }) in lastErrorDescription.errorCodeObjects" :key="description"
color="error"
:icon="userError ? 'mdi-account-alert' : 'mdi-alert'"
:icon="icon(userError, temporaryError)"
:prominent="!!userError ? true : false"
Copy link
Member

Choose a reason for hiding this comment

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

:prominent="!!userError"

>
<h4 v-if="userError">Action required</h4>
Expand Down Expand Up @@ -136,6 +136,17 @@ export default {
canLinkToSecret () {
return this.secretBindingName && this.namespace
}
},
methods: {
icon (userError, temporaryError) {
if (userError) {
return 'mdi-account-alert'
}
if (temporaryError) {
return 'mdi-clock-alert'
}
Copy link
Member

Choose a reason for hiding this comment

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

do you think we really need an own icon for temporary errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. I thought has we highlight errors with known error codes, this could improve the ability to find out what is going on with your clusters faster. We can discuss this. As we also use this icon as an indication for cluster expiration it could irritate some users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a screenshot that shows the icon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the dedicated icon

return 'mdi-alert'
}
}
}
</script>
Expand Down
13 changes: 7 additions & 6 deletions frontend/src/components/ShootStatus.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ SPDX-License-Identifier: Apache-2.0
<div v-on="on">
<v-progress-circular v-if="showProgress" class="vertical-align-middle cursor-pointer" :size="27" :width="3" :value="shootLastOperation.progress" :color="color" :rotate="-90">
<v-icon v-if="isShootStatusHibernated" class="vertical-align-middle progress-icon" :color="color">mdi-sleep</v-icon>
<v-icon v-else-if="isUserError" class="vertical-align-middle progress-icon-user-error" color="error">mdi-account-alert</v-icon>
<v-icon v-else-if="isUserError" class="vertical-align-middle progress-icon-user-error" :color="color">mdi-account-alert</v-icon>
<v-icon v-else-if="isShootLastOperationTypeDelete" class="vertical-align-middle progress-icon" :color="color">mdi-delete</v-icon>
<v-icon v-else-if="isTypeCreate" class="vertical-align-middle progress-icon" :color="color">mdi-plus</v-icon>
<v-icon v-else-if="isTypeReconcile && !isError" class="vertical-align-middle progress-icon-check" :color="color">mdi-check</v-icon>
Expand All @@ -33,8 +33,10 @@ SPDX-License-Identifier: Apache-2.0
<div>
<span class="font-weight-bold">{{tooltip.title}}</span>
<span v-if="tooltip.progress" class="ml-1">({{tooltip.progress}}%)</span>
<div v-for="({ shortDescription }) in tooltip.userErrorCodeObjects" :key="shortDescription">
<v-icon class="mr-1" color="white" small>mdi-account-alert</v-icon>
<div v-for="({ shortDescription, userError, temporaryError }) in tooltip.errorCodeObjects" :key="shortDescription">
<v-icon v-if="userError" class="mr-1" color="white" small>mdi-account-alert</v-icon>
<v-icon v-else-if="temporaryError" class="mr-1" color="white" small>mdi-clock-alert</v-icon>
<v-icon v-else class="mr-1" color="white" small>mdi-alert</v-icon>
Copy link
Member

Choose a reason for hiding this comment

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

Only the icon name is different. Why not a method like above?

<span class="font-weight-bold text--lighten-2">{{shortDescription}}</span>
</div>
</div>
Expand All @@ -43,7 +45,7 @@ SPDX-License-Identifier: Apache-2.0
<span v-if="showStatusText" class="d-flex align-center ml-2">{{statusTitle}}</span>
</div>
<template v-if="showStatusText">
<div v-for="({ description }) in tooltip.userErrorCodeObjects" :key="description">
<div v-for="({ description }) in tooltip.errorCodeObjects" :key="description">
<div class="font-weight-bold error--text wrap">{{description}}</div>
</div>
</template>
Expand All @@ -62,7 +64,6 @@ SPDX-License-Identifier: Apache-2.0
<script>
import join from 'lodash/join'
import map from 'lodash/map'
import filter from 'lodash/filter'

import GPopper from '@/components/GPopper'
import RetryOperation from '@/components/RetryOperation'
Expand Down Expand Up @@ -150,7 +151,7 @@ export default {
return {
title: this.statusTitle,
progress: this.showProgress ? this.shootLastOperation.progress : undefined,
userErrorCodeObjects: filter(objectsFromErrorCodes(this.allErrorCodes), { userError: true })
errorCodeObjects: objectsFromErrorCodes(this.allErrorCodes)
}
},
operationType () {
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/router/guards.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ function ensureDataLoaded (store, localStorage) {
const defaultFilter = {
onlyShootsWithIssues: isAdmin,
progressing: true,
userIssues: isAdmin,
noOperatorAction: isAdmin,
deactivatedReconciliation: isAdmin,
hideTicketsWithLabel: isAdmin
}
Expand Down
8 changes: 5 additions & 3 deletions frontend/src/store/modules/shoots/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import {
randomMaintenanceBegin,
maintenanceWindowWithBeginAndTimezone
} from '@/utils'
import { isUserError, errorCodesFromArray } from '@/utils/errorCodes'
import { isUserError, isTemporaryError, errorCodesFromArray } from '@/utils/errorCodes'
import find from 'lodash/find'

const uriPattern = /^([^:/?#]+:)?(\/\/[^/?#]*)?([^?#]*)(\?[^#]*)?(#.*)?/
Expand Down Expand Up @@ -287,13 +287,15 @@ function setFilteredItems (state, rootState) {
}
items = filter(items, predicate)
}
if (get(state, 'shootListFilters.userIssues', false)) {
if (get(state, 'shootListFilters.noOperatorAction', false)) {
const predicate = item => {
const lastErrors = get(item, 'status.lastErrors', [])
const allLastErrorCodes = errorCodesFromArray(lastErrors)
const conditions = get(item, 'status.conditions', [])
const allConditionCodes = errorCodesFromArray(conditions)
return !isUserError(allLastErrorCodes) && !isUserError(allConditionCodes)
const noUserError = !isUserError(allLastErrorCodes) && !isUserError(allConditionCodes)
const noTemporaryError = !isTemporaryError(allLastErrorCodes)
return noUserError && noTemporaryError
}
items = filter(items, predicate)
}
Expand Down
22 changes: 22 additions & 0 deletions frontend/src/utils/errorCodes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ export function isUserError (errorCodesArray) {
return find(objectsFromErrorCodes(errorCodesArray), { userError: true })
}

export function isTemporaryError (errorCodesArray) {
if (isEmpty(errorCodesArray)) {
return false
}

return find(objectsFromErrorCodes(errorCodesArray), { temporaryError: true })
}

export function objectsFromErrorCodes (errorCodesArray) {
return map(errorCodesArray, code => get(errorCodes, code, { code }))
}
Expand All @@ -34,46 +42,60 @@ const errorCodes = {
ERR_INFRA_UNAUTHORIZED: {
shortDescription: 'Invalid Credentials',
description: 'Invalid cloud provider credentials.',
temporaryError: false,
userError: true,
infraAccountError: true
},
ERR_INFRA_INSUFFICIENT_PRIVILEGES: {
shortDescription: 'Insufficient Privileges',
description: 'Cloud provider credentials have insufficient privileges.',
temporaryError: false,
userError: true,
infraAccountError: true
},
ERR_INFRA_QUOTA_EXCEEDED: {
shortDescription: 'Quota Exceeded',
description: 'Cloud provider quota exceeded. Please request limit increases.',
temporaryError: false,
userError: true,
infraAccountError: true
},
ERR_INFRA_DEPENDENCIES: {
shortDescription: 'Infrastructure Dependencies',
description: 'Infrastructure operation failed as unmanaged resources exist in your cloud provider account. Please delete all manually created resources related to this Shoot.',
temporaryError: false,
userError: true,
infraAccountError: true
},
ERR_CLEANUP_CLUSTER_RESOURCES: {
shortDescription: 'Cleanup Cluster',
description: 'Cleaning up the cluster failed as some resource are stuck in deletion. Please remove these resources properly or a forceful deletion will happen if this error persists.',
temporaryError: false,
userError: true
},
ERR_INFRA_RESOURCES_DEPLETED: {
shortDescription: 'Infrastructure Resources Depleted',
description: 'The underlying infrastructure provider proclaimed that it does not have enough resources to fulfill your request at this point in time. You might want to wait or change your shoot configuration.',
temporaryError: false,
userError: true,
infraAccountError: true
},
ERR_CONFIGURATION_PROBLEM: {
shortDescription: 'Configuration Problem',
description: 'There is a configuration problem that is most likely caused by your Shoot specification. Please double-check the error message and resolve the problem.',
temporaryError: false,
userError: true
},
ERR_RETRYABLE_CONFIGURATION_PROBLEM: {
shortDescription: 'Configuration Problem',
description: 'There is a configuration problem. Please double-check the error message and resolve the problem.',
temporaryError: false,
userError: true
},
ERR_INFRA_REQUEST_THROTTLING: {
shortDescription: 'Rate Limit Exceeded',
description: 'Cloud provider rate limit exceeded. The operation will be retried automatically.',
temporaryError: true,
userError: false
}
}
21 changes: 16 additions & 5 deletions frontend/src/views/ShootList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,13 @@ export default {
const key = value
await this.setShootListFilter({ filter: key, value: !this.getShootListFilters[key] })

this.$localStorage.setObject('project/_all/shoot-list/filter', pick(this.getShootListFilters, ['onlyShootsWithIssues', 'progressing', 'userIssues', 'deactivatedReconciliation', 'hideTicketsWithLabel']))
this.$localStorage.setObject('project/_all/shoot-list/filter', pick(this.getShootListFilters, [
'onlyShootsWithIssues',
'progressing',
'noOperatorAction',
'deactivatedReconciliation',
'hideTicketsWithLabel'
]))

if (key === 'onlyShootsWithIssues') {
this.subscribeShoots()
Expand Down Expand Up @@ -461,10 +467,15 @@ export default {
disabled: this.filtersDisabled
},
{
text: 'Hide user issues',
value: 'userIssues',
selected: this.isFilterActive('userIssues'),
text: 'Hide no operator action required issues',
value: 'noOperatorAction',
selected: this.isFilterActive('noOperatorAction'),
hidden: this.projectScope || !this.isAdmin,
helpTooltip: [
'Hide clusters that do not require action by an operator',
'- Clusters with user issues',
'- Clusters with temporary issues that will be retried automatically'
],
disabled: this.filtersDisabled
},
{
Expand Down Expand Up @@ -516,7 +527,7 @@ export default {
if (this.isFilterActive('progressing')) {
subtitle.push('Progressing Clusters')
}
if (this.isFilterActive('userIssues')) {
if (this.isFilterActive('noOperatorAction')) {
subtitle.push('User Errors')
}
if (this.isFilterActive('deactivatedReconciliation')) {
Expand Down