gyfora commented on code in PR #749: URL: https://github.com/apache/flink-kubernetes-operator/pull/749#discussion_r1452399899
########## flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/utils/ConditionUtils.java: ########## @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.kubernetes.operator.api.utils; + +import io.fabric8.kubernetes.api.model.Condition; +import io.fabric8.kubernetes.api.model.ConditionBuilder; + +import java.text.SimpleDateFormat; +import java.util.Date; + +/** Status of CR. */ Review Comment: Incorrect javadoc ########## flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/utils/ConditionUtils.java: ########## @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.kubernetes.operator.api.utils; + +import io.fabric8.kubernetes.api.model.Condition; +import io.fabric8.kubernetes.api.model.ConditionBuilder; + +import java.text.SimpleDateFormat; +import java.util.Date; + +/** Status of CR. */ +public class ConditionUtils { + + public static Condition ready(final String message) { + return crCondition("Ready", "True", message, "Ready"); + } + + public static Condition notReady(final String message) { + return crCondition("Ready", "False", message, "Progressing"); + } + + public static Condition error(final String message) { + return crCondition("Error", "True", message, "The job terminally failed"); Review Comment: I don't think error means that the job terminally failed in all cases. There are also operator side errors like reconciliation problems etc. ########## flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/CommonStatus.java: ########## @@ -101,4 +108,67 @@ public ResourceLifecycleState getLifecycleState() { * loop immediately. For example autoscaler overrides have changed and we need to apply them. */ @JsonIgnore @Internal private boolean immediateReconciliationNeeded = false; + + public List<Condition> getConditions() { + switch (getLifecycleState()) { + case CREATED: + updateConditionIfNotExist( + conditions, + ConditionUtils.notReady( + "The resource was created in Kubernetes but not yet handled by the operator")); + break; + case SUSPENDED: + updateConditionIfNotExist( + conditions, + ConditionUtils.notReady("The resource (job) has been suspended")); + break; + case UPGRADING: + updateConditionIfNotExist( + conditions, ConditionUtils.notReady("The resource is being upgraded")); + break; + case DEPLOYED: + updateConditionIfNotExist( + conditions, + ConditionUtils.ready( + "The resource is deployed, but it’s not yet considered to be stable and might be rolled back in the future")); + break; + case ROLLING_BACK: + updateConditionIfNotExist( + conditions, + ConditionUtils.notReady( + "The resource is being rolled back to the last stable spec")); + break; + case ROLLED_BACK: + updateConditionIfNotExist( + conditions, + ConditionUtils.ready("The resource is deployed with the last stable spec")); Review Comment: This looks like a lot of duplicated code and copy-pasted strings from the `ResourceLifeCycleState` , I think we should add this conversion to the enum (or a utility class using the enum directly) ########## flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/CommonStatus.java: ########## @@ -101,4 +108,67 @@ public ResourceLifecycleState getLifecycleState() { * loop immediately. For example autoscaler overrides have changed and we need to apply them. */ @JsonIgnore @Internal private boolean immediateReconciliationNeeded = false; + + public List<Condition> getConditions() { + switch (getLifecycleState()) { + case CREATED: + updateConditionIfNotExist( + conditions, + ConditionUtils.notReady( + "The resource was created in Kubernetes but not yet handled by the operator")); + break; + case SUSPENDED: + updateConditionIfNotExist( + conditions, + ConditionUtils.notReady("The resource (job) has been suspended")); + break; + case UPGRADING: + updateConditionIfNotExist( + conditions, ConditionUtils.notReady("The resource is being upgraded")); + break; + case DEPLOYED: + updateConditionIfNotExist( + conditions, + ConditionUtils.ready( + "The resource is deployed, but it’s not yet considered to be stable and might be rolled back in the future")); + break; + case ROLLING_BACK: + updateConditionIfNotExist( + conditions, + ConditionUtils.notReady( + "The resource is being rolled back to the last stable spec")); + break; + case ROLLED_BACK: + updateConditionIfNotExist( + conditions, + ConditionUtils.ready("The resource is deployed with the last stable spec")); + break; + case FAILED: + updateConditionIfNotExist(conditions, ConditionUtils.error("failed")); + break; + case STABLE: + updateConditionIfNotExist( + conditions, + ConditionUtils.ready( + "The resource deployment is considered to be stable and won’t be rolled back")); + break; + } + + return conditions; + } + + private void updateConditionIfNotExist(List<Condition> conditions, Condition newCondition) { + if (conditions.isEmpty()) { + conditions.add(newCondition); + } + if (conditions.stream() + .noneMatch(condition -> condition.getType().equals(newCondition.getType()))) { + conditions.add(newCondition); + } else if (conditions.removeIf( + condition -> + !(condition.getReason().equals(newCondition.getReason()) + && condition.getMessage().equals(newCondition.getMessage())))) { + conditions.add(newCondition); + } Review Comment: Can you please explain what this is supposed to do exactly (and also add some docs to the code)? I am a bit confused by the logic ########## flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/utils/ConditionUtils.java: ########## @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.kubernetes.operator.api.utils; + +import io.fabric8.kubernetes.api.model.Condition; +import io.fabric8.kubernetes.api.model.ConditionBuilder; + +import java.text.SimpleDateFormat; +import java.util.Date; + +/** Status of CR. */ +public class ConditionUtils { + + public static Condition ready(final String message) { + return crCondition("Ready", "True", message, "Ready"); + } + + public static Condition notReady(final String message) { + return crCondition("Ready", "False", message, "Progressing"); + } + + public static Condition error(final String message) { + return crCondition("Error", "True", message, "The job terminally failed"); + } + + public static Condition crCondition( + final String type, final String status, final String message, final String reason) { + return new ConditionBuilder() + .withType(type) + .withStatus(status) + .withMessage(message) + .withReason(reason) + .withLastTransitionTime( + new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'").format(new Date())) Review Comment: Wouldn't `Instant.now().toString()` work? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
