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

Jump to bottom

feat: compute reservation create shared #3832

Closed
wants to merge 10 commits into from
Closed

feat: compute reservation create shared #3832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d59087a
Add compute_reservation_delete sample
gryczj Aug 30, 2024
52f5eb0
Create shared reservation
gryczj Sep 3, 2024
1013f3c
feat: compute_reservation_create_shared
gryczj Sep 4, 2024
c61419c
feat: add compute_reservation_consumer_projects_update
gryczj Sep 11, 2024
49221c3
Fix flaky tests in compute directory
gryczj Sep 13, 2024
46781a2
Changes for shared reservation
gryczj Sep 13, 2024
8dd50c4
Changes for shared reservation
gryczj Sep 16, 2024
f2344a5
Refactor for compute samples
gryczj Sep 16, 2024
328d42d
Add mocks to sharedReservation, revert changes in tests
gryczj Sep 19, 2024
ed22e4a
improvement: Refactor for compute samples and their tests
gryczj Sep 16, 2024
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
18 changes: 5 additions & 13 deletions compute/disks/createComputeHyperdisk.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

'use strict';

async function main() {
async function main(diskName) {
// [START compute_hyperdisk_create]
// Import the Compute library
const computeLib = require('@google-cloud/compute');
Expand All @@ -28,14 +28,14 @@ async function main() {
const zoneOperationsClient = new computeLib.ZoneOperationsClient();

/**
* TODO(developer): Update these variables before running the sample.
* TODO(developer): Update/uncomment these variables before running the sample.
*/
// Project ID or project number of the Google Cloud project you want to use.
const projectId = await disksClient.getProjectId();
// The zone where your VM and new disk are located.
const zone = 'europe-central2-b';
// The name of the new disk
const diskName = 'disk-name';
// diskName = 'disk-name';
// The type of disk. This value uses the following format:
// "zones/{zone}/diskTypes/(hyperdisk-balanced|hyperdisk-extreme|hyperdisk-ml|hyperdisk-throughput)".
// For example: "zones/us-west3-b/diskTypes/hyperdisk-balanced"
Expand Down Expand Up @@ -78,22 +78,14 @@ async function main() {
});
}

const hyperdisk = (
await disksClient.get({
project: projectId,
zone,
disk: diskName,
})
)[0];

console.log(JSON.stringify(hyperdisk));
console.log(`Disk: ${diskName} created.`);
}

await callCreateComputeHyperdisk();
// [END compute_hyperdisk_create]
}

main().catch(err => {
main(...process.argv.slice(2)).catch(err => {
console.error(err);
process.exitCode = 1;
});
20 changes: 6 additions & 14 deletions compute/disks/createComputeHyperdiskFromPool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

'use strict';

async function main() {
async function main(diskName, storagePoolName) {
// [START compute_hyperdisk_create_from_pool]
// Import the Compute library
const computeLib = require('@google-cloud/compute');
Expand All @@ -28,16 +28,16 @@ async function main() {
const zoneOperationsClient = new computeLib.ZoneOperationsClient();

/**
* TODO(developer): Update these variables before running the sample.
* TODO(developer): Update/uncomment these variables before running the sample.
*/
// Project ID or project number of the Google Cloud project you want to use.
const projectId = await disksClient.getProjectId();
// The zone where your VM and new disk are located.
const zone = 'us-central1-a';
// The name of the new disk
const diskName = 'disk-from-pool-name';
// diskName = 'disk-from-pool-name';
// The name of the storage pool
const storagePoolName = 'storage-pool-name';
// storagePoolName = 'storage-pool-name';
// Link to the storagePool you want to use. Use format:
// https://www.googleapis.com/compute/v1/projects/{projectId}/zones/{zone}/storagePools/{storagePoolName}
const storagePool = `https://www.googleapis.com/compute/v1/projects/${projectId}/zones/${zone}/storagePools/${storagePoolName}`;
Expand Down Expand Up @@ -84,22 +84,14 @@ async function main() {
});
}

const hyperdisk = (
await disksClient.get({
project: projectId,
zone,
disk: diskName,
})
)[0];

console.log(JSON.stringify(hyperdisk));
console.log(`Disk: ${diskName} created.`);
}

await callCreateComputeHyperdiskFromPool();
// [END compute_hyperdisk_create_from_pool]
}

main().catch(err => {
main(...process.argv.slice(2)).catch(err => {
console.error(err);
process.exitCode = 1;
});
18 changes: 5 additions & 13 deletions compute/disks/createComputeHyperdiskPool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

'use strict';

async function main() {
async function main(storagePoolName) {
// [START compute_hyperdisk_pool_create]
// Import the Compute library
const computeLib = require('@google-cloud/compute');
Expand All @@ -28,14 +28,14 @@ async function main() {
const zoneOperationsClient = new computeLib.ZoneOperationsClient();

/**
* TODO(developer): Update these variables before running the sample.
* TODO(developer): Update/uncomment these variables before running the sample.
*/
// Project ID or project number of the Google Cloud project you want to use.
const projectId = await storagePoolClient.getProjectId();
// Name of the zone in which you want to create the storagePool.
const zone = 'us-central1-a';
// Name of the storagePool you want to create.
const storagePoolName = 'storage-pool-name';
// storagePoolName = 'storage-pool-name';
// The type of disk you want to create. This value uses the following format:
// "projects/{projectId}/zones/{zone}/storagePoolTypes/(hyperdisk-throughput|hyperdisk-balanced)"
const storagePoolType = `projects/${projectId}/zones/${zone}/storagePoolTypes/hyperdisk-balanced`;
Expand Down Expand Up @@ -79,22 +79,14 @@ async function main() {
});
}

const createdStoragePool = (
await storagePoolClient.get({
project: projectId,
zone,
storagePool: storagePoolName,
})
)[0];

console.log(JSON.stringify(createdStoragePool));
console.log(`Storage pool: ${storagePoolName} created.`);
}

await callCreateComputeHyperdiskPool();
// [END compute_hyperdisk_pool_create]
}

main().catch(err => {
main(...process.argv.slice(2)).catch(err => {
console.error(err);
process.exitCode = 1;
});
1 change: 1 addition & 0 deletions compute/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"chai": "^4.5.0",
"mocha": "^10.0.0",
"proxyquire": "^2.0.1",
"sinon": "^18.0.0",
"uuid": "^10.0.0"
}
}
18 changes: 5 additions & 13 deletions compute/reservations/createReservationFromProperties.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

'use strict';

async function main() {
async function main(reservationName) {
// [START compute_reservation_create]
// Import the Compute library
const computeLib = require('@google-cloud/compute');
Expand All @@ -28,14 +28,14 @@ async function main() {
const zoneOperationsClient = new computeLib.ZoneOperationsClient();

/**
* TODO(developer): Update these variables before running the sample.
* TODO(developer): Update/uncomment these variables before running the sample.
*/
// The ID of the project where you want to reserve resources.
const projectId = await reservationsClient.getProjectId();
// The zone in which to reserve resources.
const zone = 'us-central1-a';
// The name of the reservation to create.
const reservationName = 'reservation-01';
// reservationName = 'reservation-01';
// The number of VMs to reserve.
const vmsNumber = 3;
// Machine type to use for each VM.
Expand Down Expand Up @@ -96,22 +96,14 @@ async function main() {
});
}

const createdReservation = (
await reservationsClient.get({
project: projectId,
zone,
reservation: reservationName,
})
)[0];

console.log(JSON.stringify(createdReservation));
console.log(`Reservation: ${reservationName} created.`);
}

await callCreateComputeReservationFromProperties();
// [END compute_reservation_create]
}

main().catch(err => {
main(...process.argv.slice(2)).catch(err => {
console.error(err);
process.exitCode = 1;
});
16 changes: 4 additions & 12 deletions compute/reservations/createReservationInstanceTemplate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

'use strict';

async function main(location, instanceTemplateName) {
async function main(reservationName, location, instanceTemplateName) {
// [START compute_reservation_create_template]
// Import the Compute library
const computeLib = require('@google-cloud/compute');
Expand All @@ -28,14 +28,14 @@ async function main(location, instanceTemplateName) {
const zoneOperationsClient = new computeLib.ZoneOperationsClient();

/**
* TODO(developer): Update these variables before running the sample.
* TODO(developer): Update/uncomment these variables before running the sample.
*/
// The ID of the project where you want to reserve resources and where the instance template exists.
const projectId = await reservationsClient.getProjectId();
// The zone in which to reserve resources.
const zone = 'us-central1-a';
// The name of the reservation to create.
const reservationName = 'reservation-01';
// reservationName = 'reservation-01';
// The number of VMs to reserve.
const vmsNumber = 3;

Expand Down Expand Up @@ -87,15 +87,7 @@ async function main(location, instanceTemplateName) {
});
}

const createdReservation = (
await reservationsClient.get({
project: projectId,
zone,
reservation: reservationName,
})
)[0];

console.log(JSON.stringify(createdReservation));
console.log(`Reservation: ${reservationName} created.`);
}

await callCreateComputeReservationInstanceTemplate();
Expand Down
107 changes: 107 additions & 0 deletions compute/reservations/createSharedReservation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/*
* Copyright 2024 Google LLC
*
* Licensed 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
*
* https://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.
*/

'use strict';

async function main(reservationsClient, zoneOperationsClient) {
// [START compute_reservation_create_shared]
// Import the Compute library
const computeLib = require('@google-cloud/compute');
const compute = computeLib.protos.google.cloud.compute.v1;

/**
* TODO(developer): Uncomment reservationsClient and zoneOperationsClient before running the sample.
*/
// Instantiate a reservationsClient
// reservationsClient = new computeLib.ReservationsClient();
// Instantiate a zoneOperationsClient
// zoneOperationsClient = new computeLib.ZoneOperationsClient();

/**
* TODO(developer): Update these variables before running the sample.
*/
// The ID of the project where you want to reserve resources and where the instance template exists.
const projectId = await reservationsClient.getProjectId();
// The zone in which to reserve resources.
const zone = 'us-central1-a';
// The name of the reservation to create.
const reservationName = 'reservation-01';
// The number of VMs to reserve.
const vmsNumber = 3;
// The name of an existing instance template.
const instanceTemplateName = 'global-instance-template-name';
// The location of the instance template.
const location = 'global';

async function callCreateComputeSharedReservation() {
// Create reservation for 3 VMs in zone us-central1-a by specifying a instance template.
const specificReservation = new compute.AllocationSpecificSKUReservation({
count: vmsNumber,
sourceInstanceTemplate: `projects/${projectId}/${location}/instanceTemplates/${instanceTemplateName}`,
});

// Create share settings. Share reservation with one customer project.
const shareSettings = new compute.ShareSettings({
shareType: 'SPECIFIC_PROJECTS',
projectMap: {
// The IDs of projects that can consume this reservation. You can include up to 100 consumer projects.
// These projects must be in the same organization as the owner project.
// Don't include the owner project. By default, it is already allowed to consume the reservation.
consumer_project_id: {
projectId: 'consumer_project_id',
},
},
});

// Create a reservation.
const reservation = new compute.Reservation({
name: reservationName,
specificReservation,
specificReservationRequired: true,
shareSettings,
});

const [response] = await reservationsClient.insert({
project: projectId,
reservationResource: reservation,
zone,
});

let operation = response.latestResponse;

// Wait for the create reservation operation to complete.
while (operation.status !== 'DONE') {
[operation] = await zoneOperationsClient.wait({
operation: operation.name,
project: projectId,
zone: operation.zone.split('/').pop(),
});
}

console.log(`Reservation: ${reservationName} created.`);
}

await callCreateComputeSharedReservation();
// [END compute_reservation_create_shared]
}

module.exports = main;

// TODO(developer): Uncomment below lines before running the sample.
Copy link
Member

Choose a reason for hiding this comment

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

Hi @gryczj. My comment here will apply to both modules and to the test. What really are we looking for to know if the sample did what it was intended? We are not unit testing production APIs, we simply expect that our call to an API will have the intended result. What is that? A resource ID? Not returning a failed status code or catching an exception, etc? If you were a developer using this API in production code, what would you expect -- that's the check you could demonstrate right here. The unit test probably doesn't really need mocks and all the extra complexity; isn't it enough, really, if main doesn't throw (the process doesn't exit with a non-zero exit code)? Our samples shouldn't themselves be APIs, so no need to export anything out of this module.

Copy link
Member

Choose a reason for hiding this comment

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

I just want to add your PRs have been great! To clarify here, the goal is that a sample should fully encapsulate the demonstration of what a developer needs to know to use our APIs successfully in their own production code. How is the API intended to be used? Our tests don't need to confirm that APIs work as expected, only that the sample works (we don't want broken samples in our docs for an unexpected reason, but we do expect that service teams have already exhaustively tested their services). Hope this helps!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@subfuzion thank you for your comments and review :) I introduced mocks here because to create shared reservation special permission are required and also another project, that we want to use as consumer and share this reservation with. I can test this sample locally with my credentials, but I have no knowledge about our test environment and projects, which I can use here as an example in tests and this sample will throw errors.
I understand your point. All new changes in the implementation were made to allow me to use mocks and fake clients in tests and on the other hand allow user to use real clients if they want to.

Copy link
Contributor

Choose a reason for hiding this comment

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

@subfuzion As @gryczj wrote, the shared reservation sample would require a separate project, with a change to organization policy to allow this project to share the reservation with other projects.

I made an "executive" call for the team that writes those samples for Python, Java, Go and Node to use mocks for those samples as a kind of an exception to the rule.

My arguments:

  • We don't really do tests that span multiple projects.
  • It'd be very easy to forget about this additional project and in a year or two we'd have to rediscover the whole thing.
  • I know that Python has separate projects for every version of the language - having tests that span multiple projects that require org policy updates would make it harder to start up a new project for new version.

We acknowledge the value of proper testing and that's why mocks were picked as a middle ground between skipping tests for those samples and writing fully functional tests that would make use of multiple projects.

Let me know if I managed to convince you or not :)

Copy link
Member

@subfuzion subfuzion Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the added context. I still don't see the need to mock console or treat the sample like its library code and have it export a function for testing, and my points about overall test simplification still remain. I'd like to surface this up to the rest of our team for discussion, so can you please address the points I've raised specifically?

Also, overall test flakiness still seems to be a problem, such as the compute hyperdisk failures for #3834 (which I merged so we can consolidate efforts on a single PR).

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Please feel free to ping me directly on internal chat: tonypujals@

// main(...process.argv.slice(2)).catch(err => {
// console.error(err);
// process.exitCode = 1;
// });
Loading
Loading