-
Notifications
You must be signed in to change notification settings - Fork 62
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.
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
fix: removing only networks started by hedera instead of global prune #686
Changes from 2 commits
7dce9a2
728fc89
1c19719
a77e477
851de02
358a4b6
4e8569b
6cfd740
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,16 @@ export const CONTAINERS = [ | |
}, | ||
]; | ||
|
||
/** | ||
* Names of all the networks created by Docker Compose in this repository. | ||
* Make sure to keep them up to date to always be able to remove them all using this service. | ||
*/ | ||
export const NETWORK_NAMES = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. QQ: can’t we get this dynamically? It’s a pain to have to update this constant every time we make modifications to the docker-compose configurations. Also, one might not know this and may introduce bugs if he forgets to update this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw, why not use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @victor-yanev it's called, but this is an extra cleanup step. Alternatively to avoid the need for const There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Understood, it will now function as described. |
||
'hedera-cloud-storage', | ||
'hedera-mirror-node', | ||
'hedera-network-node-bridge', | ||
]; | ||
|
||
export const CONSENSUS_NODE_LABEL = "network-node"; | ||
export const MIRROR_NODE_LABEL = "mirror-node-rest"; | ||
export const RELAY_LABEL = "json-rpc-relay"; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
/*- | ||
* | ||
* Hedera Local Node | ||
* | ||
* Copyright (C) 2024 Hedera Hashgraph, 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 | ||
* | ||
* 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. | ||
* | ||
*/ | ||
|
||
import shell from 'shelljs'; | ||
import { IS_WINDOWS, NETWORK_NAMES } from "../constants"; | ||
|
||
/** | ||
* Checks if the given string is a valid Docker network ID. | ||
* A valid Docker network ID is a 12-character hexadecimal string. | ||
* | ||
* @param {string} id - The string to be validated as a Docker network ID. | ||
* @returns {boolean} - Returns true if the string is a valid Docker network ID, false otherwise. | ||
* | ||
* @example | ||
* const id = "89ded1eca1d5"; | ||
* console.log(isCorrectDockerId(id)); // Output: true | ||
* | ||
* @example | ||
* const invalidId = "invalidID123"; | ||
* console.log(isCorrectDockerId(invalidId)); // Output: false | ||
*/ | ||
const isCorrectDockerId = (id: string) => id.trim() !== '' && /^[a-f0-9]{12}$/.test(id); | ||
|
||
/** | ||
* Provides utility methods for safe networks removal | ||
*/ | ||
export class SafeDockerNetworkRemover { | ||
/** | ||
* Removes all the networks started by docker compose. | ||
*/ | ||
public static removeAll() { | ||
for (const network of NETWORK_NAMES) { | ||
this.removeByName(network) | ||
} | ||
} | ||
|
||
/** | ||
* Removes one single network by its name. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is technically not true, as it will loop through all networks that match that name filter and remove them, so for example an argument of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @isavov You are correct. I have updated the comment and added a remark to the README file regarding the functionality of our script. Currently, it will remove all networks with the “hedera-” prefix. |
||
*/ | ||
public static removeByName(name: string) { | ||
const result = shell.exec(`docker network ls --filter name=${name} --format "{{.ID}}"`); | ||
if (!result || result.stderr !== '') { | ||
return; | ||
} | ||
for (const id of result.stdout.split('\n').filter(isCorrectDockerId)) { | ||
shell.exec(`docker network rm ${id} -f 2>${IS_WINDOWS ? 'null' : '/dev/null'}`); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
/*- | ||
* | ||
* Hedera Local Node | ||
* | ||
* Copyright (C) 2024 Hedera Hashgraph, 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 | ||
* | ||
* 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. | ||
* | ||
*/ | ||
|
||
import { expect } from 'chai'; | ||
import sinon from 'sinon'; | ||
import { SafeDockerNetworkRemover } from '../../../src/utils/SafeDockerNetworkRemover'; | ||
import { IS_WINDOWS, NETWORK_NAMES } from '../../../src/constants'; | ||
import { getTestBed } from '../testBed'; | ||
import yaml, {load, loadAll} from 'js-yaml'; | ||
import fs, {createReadStream, readdirSync, readFile, readFileSync} from 'fs'; | ||
import { join } from "path"; | ||
|
||
describe('SafeDockerNetworkRemover', () => { | ||
let shellExecStub: sinon.SinonStub; | ||
|
||
before(() => { | ||
let { | ||
shellStubs | ||
} = getTestBed({ | ||
workDir: 'testDir', | ||
}); | ||
|
||
shellExecStub = shellStubs.shellExecStub; | ||
}); | ||
|
||
describe('removeByName', () => { | ||
it('should remove the network when a valid ID is returned', () => { | ||
shellExecStub | ||
.withArgs('docker network ls --filter name=hedera-cloud-storage --format "{{.ID}}"') | ||
.returns({ stderr: '', stdout: '89ded1eca1d5\n' }); | ||
|
||
shellExecStub | ||
.withArgs('docker network rm 89ded1eca1d5 -f 2>null') | ||
.returns({ stderr: '', stdout: '' }); | ||
|
||
SafeDockerNetworkRemover.removeByName('hedera-cloud-storage'); | ||
|
||
expect(shellExecStub.calledWith('docker network ls --filter name=hedera-cloud-storage --format "{{.ID}}"')).to.be.true; | ||
expect(shellExecStub.calledWith(`docker network rm 89ded1eca1d5 -f 2>${IS_WINDOWS ? 'null' : '/dev/null'}`)).to.be.true; | ||
}); | ||
|
||
it('should not attempt to remove network if no valid ID is returned', () => { | ||
shellExecStub | ||
.withArgs('docker network ls --filter name=hedera-cloud-storage --format "{{.ID}}"') | ||
.returns({ stderr: '', stdout: 'invalidID123\n' }); | ||
|
||
SafeDockerNetworkRemover.removeByName('hedera-cloud-storage'); | ||
|
||
expect(shellExecStub.calledWith('docker network ls --filter name=hedera-cloud-storage --format "{{.ID}}"')).to.be.true; | ||
expect(shellExecStub.calledWith('docker network rm invalidID123 -f 2>null')).to.be.false; | ||
}); | ||
|
||
it('should handle shell errors gracefully', () => { | ||
shellExecStub | ||
.withArgs('docker network ls --filter name=hedera-cloud-storage --format "{{.ID}}"') | ||
.returns({ stderr: 'some error', stdout: '' }); | ||
|
||
SafeDockerNetworkRemover.removeByName('hedera-cloud-storage'); | ||
|
||
expect(shellExecStub.calledWith('docker network ls --filter name=hedera-cloud-storage --format "{{.ID}}"')).to.be.true; | ||
expect(shellExecStub.calledWith('docker network rm 89ded1eca1d5 -f 2>null')).to.be.false; | ||
}); | ||
}); | ||
|
||
describe('removeAll', () => { | ||
it('should call removeByName for each network', () => { | ||
const removeByNameStub = sinon.stub(SafeDockerNetworkRemover, 'removeByName'); | ||
SafeDockerNetworkRemover.removeAll(); | ||
expect(removeByNameStub.calledWith('hedera-cloud-storage')).to.be.true; | ||
removeByNameStub.restore(); | ||
}); | ||
it('check if all of the networks from composer yaml files are listed in the NETWORK_NAMES const', () => { | ||
const relativePath = '../../..'; | ||
const files = readdirSync(join(__dirname, relativePath)).filter(name => /^docker-compose.*\.yml$/.test(name)); | ||
let actual = []; | ||
for (const file of files) { | ||
const data = readFileSync(join(__dirname, `${relativePath}/${file}`)); | ||
const config = load(data.toString()); | ||
actual = [...actual, ...Object.values(config.networks || {}).map((network: any) => network.name.trim())]; | ||
} | ||
expect([...new Set(actual)].sort().join(',')).to.equal( | ||
NETWORK_NAMES.sort().join(','), | ||
`Make sure that no network is missing in NETWORK_NAMES const. It won't be removed by 'npm run stop' otherwise`, | ||
); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe for consistency's sake all networks should have a -bridge suffix, or none of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@isavov
I chose the latter option since there is only one network with the “bridge” suffix and a few without it.
I have only updated the name used by Docker, not the Docker Compose level alias, as changing the alias seemed to be outside the current scope.