Skip to content

Commit

Permalink
Merge #11471 #11559
Browse files Browse the repository at this point in the history
11471: Improve `bors` merge reliability: Windows smoke tests are advisory and run with reduced parallelism r=AaronFriel a=AaronFriel

The `continue-on-error` flag is set to true whenever the matrix platform contains "windows" for smoke tests. This does mean these jobs will allow failures and proceed, however this should substantially reduce the false negatives due to the behavior of the CI platform. Per the response from GitHub support ticket on my account:

> I've confirmed that our engineering team has an open backlog item focused around "protecting" the provisioning service in cases of resource starvation. I believe this will address your exact concerns so that high resource consumption does not result in an abandoned job.
> 
> In the meantime, there unfortunately is not much capability exposed for affecting priority on the VM. Until improvements are implemented, our team still recommends trying to reduce load on the CPU at the point(s) in the workflow where the job consistently is getting abandoned.

I hope that setting this option, reducing Windows test parallelism, and setting timeouts appropriately will help improve CI reliability. 

Our CI jobs typically complete in 35 minutes, any longer indicates high likelihood that the runner has failed.

11559: fix(sdk/python): Allow for duplicate output values in python programs r=kpitzen a=kpitzen

I used the node SDK as inspiration for this change - there were a few things we were doing differently in python (specifically handling all cases in the outer layer of `massage` rather than allowing for more fine-grained control in a `massage_complex` function like in node.  We also take the stack concept from the node SDK to resolve the immediate issue of duplicate outputs.

<!--- 
Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation.
-->

# Description

<!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. -->

Fixes #11133 

## Checklist

<!--- Please provide details if the checkbox below is to be left unchecked. -->
- [x] I have added tests that prove my fix is effective or that my feature works
<!--- 
User-facing changes require a CHANGELOG entry.
-->
- [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the Pulumi Service,
then the service should honor older versions of the CLI where this change would not exist.
You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version
  <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. -->


Co-authored-by: Aaron Friel <mayreply@aaronfriel.com>
Co-authored-by: Kyle Pitzen <kylepitzen@Kyles-MBP.fios-router.home>
  • Loading branch information
3 people committed Dec 8, 2022
3 parents 43cfc49 + 8457dfb + 592dc92 commit f43f549
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 27 deletions.
7 changes: 7 additions & 0 deletions .github/workflows/ci-run-test.yml
Expand Up @@ -50,6 +50,11 @@ on:
"nodejs": "14.x",
"python": "3.9.x"
}
continue-on-error:
description: "Whether to continue running the job if the step fails"
required: false
default: false
type: boolean


defaults:
Expand Down Expand Up @@ -84,6 +89,8 @@ jobs:

runs-on: ${{ inputs.platform }}

timeout-minutes: 60
continue-on-error: ${{ inputs.continue-on-error }}
steps:
- name: "Windows cache workaround"
# https://github.com/actions/cache/issues/752#issuecomment-1222415717
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
Expand Up @@ -293,6 +293,7 @@ jobs:
name: Smoke Test${{ matrix.platform && '' }}
needs: [matrix, build-binaries, build-sdks]
if: ${{ needs.matrix.outputs.smoke-test-matrix != '{}' }}
# alow jobs to fail if the platform contains windows
strategy:
fail-fast: ${{ contains(needs.matrix.outputs.smoke-test-matrix, 'macos') }}
matrix: ${{ fromJson(needs.matrix.outputs.smoke-test-matrix) }}
Expand All @@ -309,6 +310,7 @@ jobs:
# require-build: false # TODO, remove ${{ matrix.require-build || false }}

version-set: ${{ toJson(matrix.version-set) }}
continue-on-error: ${{ contains(matrix.platform, 'windows') }}
secrets: inherit

test-collect-reports:
Expand Down
@@ -0,0 +1,4 @@
changes:
- type: fix
scope: sdk/python
description: Allows for duplicate output values in python
3 changes: 3 additions & 0 deletions scripts/get-job-matrix.py
Expand Up @@ -391,6 +391,9 @@ def get_matrix(

test_suites += run_gotestsum_ci_matrix_single_package(item, pkg_tests, tags)

if kind == JobKind.SMOKE_TEST:
platforms = list(map(lambda p: "windows-8core-2022" if p == "windows-latest" else p, platforms))

return {
"test-suite": test_suites,
"platform": platforms,
Expand Down
1 change: 1 addition & 0 deletions sdk/python/.gitignore
Expand Up @@ -6,3 +6,4 @@
.venv/
venv/
.coverage
build/
67 changes: 40 additions & 27 deletions sdk/python/lib/pulumi/runtime/stack.py
Expand Up @@ -174,54 +174,67 @@ def massage(attr: Any, seen: List[Any]):
if is_primitive(attr):
return attr

if isinstance(attr, Output):
return attr.apply(lambda v: massage(v, seen))

if isawaitable(attr):
return Output.from_input(attr).apply(lambda v: massage(v, seen))

# from this point on, we have complex objects. If we see them again, we don't want to emit them
# again fully or else we'd loop infinitely.
if reference_contains(attr, seen):
# Note: for Resources we hit again, emit their urn so cycles can be easily understood in
# the popo objects.
if isinstance(attr, Resource):
return attr.urn

return massage(attr.urn, seen)
# otherwise just emit as nothing to stop the looping.
return None

seen.append(attr)

# first check if the value is an actual dictionary. If so, massage the values of it to deeply
# make sure this is a popo.
if isinstance(attr, dict):
result = {}
# Don't use attr.items() here, as it will error in the case of outputs with an `items` property.
for key in attr:
# ignore private keys
if not key.startswith("_"):
result[key] = massage(attr[key], seen)
try:
seen.append(attr)
return massage_complex(attr, seen)
finally:
popped = seen.pop()
if popped is not attr:
raise Exception("Invariant broken when processing stack outputs")

return result

if isinstance(attr, Output):
return attr.apply(lambda v: massage(v, seen))
def massage_complex(attr: Any, seen: List[Any]) -> Any:
def is_public_key(key: str) -> bool:
return not key.startswith("_")

if isawaitable(attr):
return Output.from_input(attr).apply(lambda v: massage(v, seen))
def serialize_all_keys(include: Callable[[str], bool]):
plain_object: Dict[str, Any] = {}
for key in attr.__dict__.keys():
if include(key):
plain_object[key] = massage(attr.__dict__[key], seen)
return plain_object

if isinstance(attr, Resource):
result = massage(attr.__dict__, seen)
serialized_attr = serialize_all_keys(is_public_key)

# In preview only, we mark the result with "@isPulumiResource" to indicate that it is derived
# from a resource. This allows the engine to perform resource-specific filtering of unknowns
# from output diffs during a preview. This filtering is not necessary during an update because
# all property values are known.
if is_dry_run():
result["@isPulumiResource"] = True
return result
return (
serialized_attr
if not is_dry_run()
else {**serialized_attr, "@isPulumiResource": True}
)

if hasattr(attr, "__dict__"):
# recurse on the dictionary itself. It will be handled above.
return massage(attr.__dict__, seen)
# first check if the value is an actual dictionary. If so, massage the values of it to deeply
# make sure this is a popo.
if isinstance(attr, dict):
# Don't use attr.items() here, as it will error in the case of outputs with an `items` property.
return {
key: massage(attr[key], seen) for key in attr if not key.startswith("_")
}

if hasattr(attr, "__iter__"):
return [massage(item, seen) for item in attr]

# finally, recurse through iterables, converting into a list of massaged values.
return [massage(a, seen) for a in attr]
return serialize_all_keys(is_public_key)


def reference_contains(val1: Any, seen: List[Any]) -> bool:
Expand Down
5 changes: 5 additions & 0 deletions sdk/python/lib/test/langhost/stack_output/__main__.py
Expand Up @@ -18,6 +18,9 @@ def __init__(self):
self.num = 1
self._private = 2


my_test_class_instance = TestClass()

recursive = {"a": 1}
recursive["b"] = 2
recursive["c"] = recursive
Expand All @@ -34,3 +37,5 @@ def __init__(self):
pulumi.export("output", pulumi.Output.from_input(1))
pulumi.export("class", TestClass())
pulumi.export("recursive", recursive)
pulumi.export("duplicate_output_0", my_test_class_instance)
pulumi.export("duplicate_output_1", my_test_class_instance)
Expand Up @@ -39,4 +39,6 @@ def register_resource_outputs(self, _ctx, _dry_run, _urn, ty, _name, _resource,
"output": 1.0,
"class": {"num": 1.0},
"recursive": {"a": 1.0, "b": 2.0},
"duplicate_output_0": {'num': 1.0},
"duplicate_output_1": {'num': 1.0},
}, outputs)

0 comments on commit f43f549

Please sign in to comment.